coroutine: introduce try_future #2973
Conversation
|
This sounds like it may be a good idea, but I'm having a hard time imagining in what situations I will use this new function. In the commit message, you said that: "This allows for elegant control-flow, without the if spamming that is required whe using coroutine::as_future().". Can you please give a concrete example to back this statement up? Perhaps some real code from Seastar or ScyllaDB or another Seastar project, which looks "spammy" because of its use of as_future() but will look better with the new try_future() feature? Thanks. |
This Besides being ugly and sometimes requiring restructuring of the control flow, to make the early return possible, |
|
New in v2:
If this new approach looks good, I will tend to the documentation too. |
|
So much red |
|
New in v3:
Docs are still TODO |
|
Looks good. |
|
New in v4:
|
Special awaiter which co_await:s a future and returns the wrapped result if successful, terminates the coroutine otherwise, propagating the exception directly to its waiter. If the future was successful, this is identical to co_await-ing the future directly. If the future failed, the coroutine is not resumed and instead the exception from the future is forwarded to the waiter directly and the coroutine is destroyed. The goal of this special awaiter is to provide the good ergonomics of throw -- interrupting control flow and immediately propagating the exception to the caler -- without the associated costs. The exception is propagated via the future chain, without being thrown.
|
New in v5:
|
| /// which means that it will yield if the future is ready and \ref seastar::need_preempt() | ||
| /// returns true. Use \ref coroutine::try_future_without_preemption_check | ||
| /// to disable preemption checking. | ||
| template<typename T = void> |
There was a problem hiding this comment.
I s = void necessary? I think not.
| /// terminates the coroutine otherwise, propagating the exception to the waiter. | ||
| /// | ||
| /// Same as \ref coroutine::try_future, but does not check for preemption. | ||
| template<typename T = void> |
|
Weird, a pre-existing test, testing an un-modified component started segfaulting, very consistently. The test is probably broken and somehow adding a new test to the end of the file makes it break all the time. |
|
We have a double-free in seastar/include/seastar/coroutine/exception.hh Lines 44 to 48 in acd5720 hndl.destroy() can only be called on a suspended coroutine, but here it is called on a not yet suspended one. As a result, the coroutine frame is freed twice. How was this not noticed before?
|
|
This test is also crashing when run on master, so my branch has nothing to do with it. |
|
How come? All pull requests and master regularly run the tests. |
|
Running coroutines_test locally does not reproduce. |
|
Randomly adding tests at the end did not make it reproduce. |
|
I don't understand either. FYI I'm using ScyllaDB's most recent toolchain and debug build. Doesn't reproduce in dev or release (although in CI it does reproduce with dev/release). |
|
I can reproduce with this command line on a freshly cleaned master ( |
|
BTW the use-after-free looks like this: Notice how the use/free backtraces are identical up to |
|
Seems like co_await coroutine::exception is illegal, but can't see why. |
|
Perhaps we need the same trick you used in try_future: make exception_awaiter a seastar::task, and have the task call hndl.destroy() instead of await_suspend(). Not based on real understanding, just guesses. |
|
btw, it would be sad, since it's converting immediate execution to scheduled execution. |
|
It's also possible this is just a gdb bug. |
|
According to my read of the gcc source code, a await_suspend(hndl);
goto return_with_no_cleanup;
resume:
auto y = await_resume();
[...]
return_with_no_cleanup:
// return without calling destructorsAccording to this, calling |
Actually, this crash is the reason I chose this route for |
|
One more observation: the crash only happens if there was a previous |
|
Here's a reproducer: |
|
In the reproducer, there is no previous co_await. |
|
It looks like a gcc regression. In 15.1, it only detects an (expected) memory leak. Or it could be that gcc 15.2 tightened the implementation. I'll file a gcc bug. |
|
Minimal reproducer: |
|
If this is acknowledged as a gcc bug, we can #ifdef this facility away from bad compilers. If it's really illegal, we'll have to think. |
|
Guessing gcc-mirror/gcc@b4da8ee, but will bisect. |
Confirmed |
Specialized awaiter for future which returns control to the coroutine only if the future didn't fail. If the future failed, the returned exception is returned to the waiter directly, without resuming the coroutine. This allows for elegant control-flow, without the if spamming that is required whe using coroutine::as_future().