Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Timeout#Cancel(): allow waiting for the own coroutine to finish #10250

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Al2Klimov
Copy link
Member

Especially useful not to have to drag shared pointers around in the callback.

@Al2Klimov Al2Klimov added the core/quality Improve code, libraries, algorithms, inline docs label Nov 25, 2024
@Al2Klimov Al2Klimov self-assigned this Nov 25, 2024
@Al2Klimov Al2Klimov removed their assignment Nov 25, 2024
@Al2Klimov Al2Klimov marked this pull request as ready for review November 25, 2024 11:13
{
Ptr keepAlive (this);

m_Cancelled.store(false);
m_Timer.expires_from_now(std::move(timeoutFromNow));

IoEngine::SpawnCoroutine(executor, [this, keepAlive, onTimeout](boost::asio::yield_context yc) {
auto setDone (Shared<Defer>::Make([this, keepAlive] { m_Done.Set(); }));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for this shared pointer instead of doing this inside the lambda just below?

Comment on lines +157 to +161
/**
* Cancels the timeout and waits for the own coroutine to finish.
*
* @param yc Yield Context for ASIO
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On its own, this doc comment misses context, i.e. there's no other doc comment that mentions a coroutine.

I think it would be more helpful if it instead stated the effects. Like that it waits for the involved resources to be destroyed, which includes waiting for the callback if it was already triggered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core/quality Improve code, libraries, algorithms, inline docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants