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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions lib/base/io-engine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,3 +153,14 @@ void Timeout::Cancel()
boost::system::error_code ec;
m_Timer.cancel(ec);
}

/**
* Cancels the timeout and waits for the own coroutine to finish.
*
* @param yc Yield Context for ASIO
*/
Comment on lines +157 to +161
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.

void Timeout::Cancel(boost::asio::yield_context yc)
{
Cancel();
m_Done.Wait(yc);
}
10 changes: 8 additions & 2 deletions lib/base/io-engine.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@
#ifndef IO_ENGINE_H
#define IO_ENGINE_H

#include "base/defer.hpp"
#include "base/exception.hpp"
#include "base/lazy-init.hpp"
#include "base/logger.hpp"
#include "base/shared.hpp"
#include "base/shared-object.hpp"
#include <atomic>
#include <exception>
Expand Down Expand Up @@ -172,14 +174,16 @@ class Timeout : public SharedObject

template<class Executor, class TimeoutFromNow, class OnTimeout>
Timeout(boost::asio::io_context& io, Executor& executor, TimeoutFromNow timeoutFromNow, OnTimeout onTimeout)
: m_Timer(io)
: m_Timer(io), m_Done(io)
{
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?


IoEngine::SpawnCoroutine(executor, [this, keepAlive, setDone, onTimeout](boost::asio::yield_context yc) {
if (m_Cancelled.load()) {
return;
}
Expand All @@ -204,10 +208,12 @@ class Timeout : public SharedObject
}

void Cancel();
void Cancel(boost::asio::yield_context yc);

private:
boost::asio::deadline_timer m_Timer;
std::atomic<bool> m_Cancelled;
AsioConditionVariable m_Done;
};

}
Expand Down
Loading