feat: Repeating operations for util::async #1776
Conversation
| */ | ||
| Repeat(boost::asio::io_context& ioc); | ||
| Repeat(auto& ctx) : timer_(ctx) |
There was a problem hiding this comment.
This makes it usable with other types of contexts such as thread_pool or strand.
There was a problem hiding this comment.
Maybe worth to leave this comment in the code.
|
|
||
| /** | ||
| * @brief Wait for the operation to get cancelled | ||
| * @warning Calling this from inside of the repeating operation yields a deadlock |
There was a problem hiding this comment.
This can be worked around with custom code instead of util::Repeat but i decided not to make it more complicated than is necessary now. We can always improve it in the future as long as the interface remains the same or does not change too much.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1776 +/- ##
===========================================
+ Coverage 71.72% 71.90% +0.18%
===========================================
Files 295 297 +2
Lines 12679 12796 +117
Branches 6499 6550 +51
===========================================
+ Hits 9094 9201 +107
+ Misses 1874 1856 -18
- Partials 1711 1739 +28 ☔ View full report in Codecov by Sentry. |
| */ | ||
| Repeat(boost::asio::io_context& ioc); | ||
| Repeat(auto& ctx) : timer_(ctx) |
There was a problem hiding this comment.
Maybe worth to leave this comment in the code.
| using DataType = std::expected<void, ExecutionError>; | ||
| using StopSourceType = typename CtxType::StopSource; | ||
|
|
||
| std::unique_ptr<util::Repeat> repeat_; |
There was a problem hiding this comment.
Repeat is not movable. But let me try with your PR #1775
| * @warning Calling this from inside of the repeating operation yields a deadlock | ||
| */ | ||
| void | ||
| wait() noexcept |
There was a problem hiding this comment.
Probably this is convenience for the whole framework but from method wait() I would expect waiting for an operation to finish.
There was a problem hiding this comment.
Yea, that's what it sort of does, waits until "finished" which in case of a repeating block of code is same as "aborted".. the naming may be not the best but it matches others in the framework
| auto mockRepeatingOp = RepeatingOperationType<std::any>{}; | ||
| EXPECT_CALL(mockRepeatingOp, wait()); | ||
| EXPECT_CALL( | ||
| mockExecutionContext, executeRepeatedly(An<std::chrono::milliseconds>(), An<std::function<std::any()>>()) |
There was a problem hiding this comment.
| mockExecutionContext, executeRepeatedly(An<std::chrono::milliseconds>(), An<std::function<std::any()>>()) | |
| mockExecutionContext, executeRepeatedly(A<std::chrono::milliseconds>(), A<std::function<std::any()>>()) |
Also why not 1 millisecond?
| ) | ||
| .WillOnce([&mockRepeatingOp] -> RepeatingOperationType<std::any> const& { return mockRepeatingOp; }); | ||
|
|
||
| auto res = ctx.executeRepeatedly(std::chrono::milliseconds(1), [] -> void { throw 0; }); |
There was a problem hiding this comment.
Why throwing? If you don't want this to be called probably strict function mock would be better.
There was a problem hiding this comment.
It's just a way to make sure this function does not actually gets called or used - that's not the intention of this test. it's the same with all other checks in this file so i just went with the same style
| { | ||
| testing::MockFunction<void()> call; | ||
|
|
||
| auto res = this->ctx.executeRepeatedly(std::chrono::milliseconds(1), [&] { call.Call(); }); |
There was a problem hiding this comment.
| auto res = this->ctx.executeRepeatedly(std::chrono::milliseconds(1), [&] { call.Call(); }); | |
| auto res = this->ctx.executeRepeatedly(std::chrono::milliseconds(1), call.AsStdFunction()); |
| { | ||
| repeat_->start(interval, std::forward<decltype(fn)>(fn)); | ||
| } | ||
|
|
There was a problem hiding this comment.
Don't you need a custom destructor here? It looks like op_ will die before repeat_ now.
Async framework needed a way to do repeating operations (think simplest cases like AmendmentBlockHandler).
This PR implements the absolute minimum, barebones repeating operations that
abort()function (but not from the user-provided repeating function)