Naios / continuable

C++14 asynchronous allocation aware futures (supporting then, exception handling, coroutines and connections)
https://naios.github.io/continuable/
MIT License
815 stars 44 forks source link

Ensure that continuables that are resolved immediately are always symmetrically transferable #68

Closed Rogiel closed 9 months ago

Rogiel commented 9 months ago

@Naios

This ensures that a continuable that is resolved using cti::make_continuable that is synchronously resolved can still be symmetrically transferred.


What was a problem?

The lack of symmetric transfer for immediately resolved continuables can cause stack overflows if the await chain is long. This is easily done if you have for example a async read/write stream that immediately resolves.

How this PR fixes the problem?

The awaiter now has an atomic coroutine_handle that will be swapped by from the next call in the continuation chain.

Check lists (check x in [ ] of list items)

Naios commented 9 months ago

@Rogiel very good PR, good catch!

I would like to merge it as soon as the review comments have been solved.

Especially I prefer to change the return type of await_suspend to bool instead of the coroutine handle.

Rogiel commented 9 months ago

I have done the requested changes.

Rogiel commented 9 months ago

I have done the requested changes.

I'm having issues with those changes now though. I am unsure why though, the tests pass, but my code that heavily uses continuable with coroutines is failing.

Rogiel commented 9 months ago

I have done the requested changes.

I'm having issues with those changes now though. I am unsure why though, the tests pass, but my code that heavily uses continuable with coroutines is failing.

Nevermind. That was a subtle bug in my code. I've added two new extra tests to ensure that this still works when the continuable is resolved asynchronously and with an exception.

Rogiel commented 9 months ago

This is not stable with the boolean. I am not exactly sure what case fails that I cannot reproduce it with the unit tests, but I have tested that there's no such issue using the atomic coroutine_handle.

Rogiel commented 9 months ago

I suspect that has to do with the fact that we have 3 states with the coroutine handle: null, noop_handle and the actual handle. With the boolean, we can't emulate the same.

We don't have the issue with a null handle using the handle, since null is only assigned when we exchange on the return statement. I could try to get the same working with an enum with 3 states instead.

Rogiel commented 9 months ago

I got this working with the state enum. Let me know what you think of this approach.

Naios commented 9 months ago

@Rogiel I'm not sure why it does not work with a single bool flag, that tracks whether the coroutine_handle has been resumed or not. Theoretically it should work with a single bool. Is it possible that await_suspend is called repetively on the same object?

In this case it would make sense to add an assert() that at the beginning of await_suspend whether the atomic is in the initial state.

EDIT: I Have noticed that it cannot work wih a single bool since we need to track inside the continuation whether we are still inside the outer scope. So your changes perfectly make sense and it cannot be solved with a single bool.