-
Notifications
You must be signed in to change notification settings - Fork 51
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
Fix coroutine continuation cleanup #181
Merged
li-feng-sc
merged 4 commits into
Snapchat:main
from
jb-gcx:gcx/fix-coroutine-continuation
Jul 18, 2024
Merged
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,8 @@ | |
|
||
#pragma once | ||
|
||
#include "expected.hpp" | ||
|
||
#include <atomic> | ||
#include <functional> | ||
#include <memory> | ||
|
@@ -364,34 +366,63 @@ class Future { | |
return true; | ||
} | ||
|
||
template<typename ConcretePromise> | ||
struct PromiseTypeBase { | ||
Promise<T> _promise; | ||
std::optional<djinni::expected<T, std::exception_ptr>> _result{}; | ||
|
||
struct FinalAwaiter { | ||
constexpr bool await_ready() const noexcept { | ||
return false; | ||
} | ||
bool await_suspend(std::coroutine_handle<ConcretePromise> finished) const noexcept { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's use the alias There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done 👍 |
||
auto& promise_type = finished.promise(); | ||
if (*promise_type._result) { | ||
if constexpr (std::is_void_v<T>) { | ||
promise_type._promise.setValue(); | ||
} else { | ||
promise_type._promise.setValue(std::move(**promise_type._result)); | ||
} | ||
} else { | ||
promise_type._promise.setException(std::move(promise_type._result->error())); | ||
} | ||
return false; | ||
} | ||
constexpr void await_resume() const noexcept {} | ||
}; | ||
|
||
detail::SuspendNever initial_suspend() { return {}; } | ||
detail::SuspendNever final_suspend() noexcept { return {}; } | ||
constexpr detail::SuspendNever initial_suspend() const noexcept { return {}; } | ||
FinalAwaiter final_suspend() const noexcept { return {}; } | ||
|
||
Future<T> get_return_object() noexcept { | ||
return _promise.getFuture(); | ||
} | ||
void unhandled_exception() { | ||
_promise.setException(std::current_exception()); | ||
_result.emplace(djinni::unexpect, std::current_exception()); | ||
} | ||
}; | ||
template <typename U> | ||
struct PromiseType: PromiseTypeBase{ | ||
template <typename V> | ||
|
||
struct PromiseType: PromiseTypeBase<PromiseType>{ | ||
template <typename V, typename = std::enable_if_t<std::is_convertible_v<V, T>>> | ||
void return_value(V&& value) { | ||
this->_promise.setValue(std::forward<V>(value)); | ||
this->_result.emplace(std::forward<V>(value)); | ||
} | ||
void return_value(T&& value) { | ||
this->_result.emplace(std::move(value)); | ||
} | ||
void return_value(const T& value) { | ||
this->_result.emplace(value); | ||
} | ||
}; | ||
using promise_type = PromiseType<T>; | ||
using promise_type = PromiseType; | ||
#endif | ||
}; | ||
|
||
#if defined(DJINNI_FUTURE_HAS_COROUTINE_SUPPORT) | ||
template<> template<> struct Future<void>::PromiseType<void>:PromiseTypeBase { | ||
template<> | ||
struct Future<void>::PromiseType : PromiseTypeBase<PromiseType> { | ||
void return_void() { | ||
this->_promise.setValue(); | ||
_result.emplace(); | ||
} | ||
}; | ||
#endif | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
#import <Foundation/Foundation.h> | ||
#import <XCTest/XCTest.h> | ||
|
||
#include <Future.hpp> | ||
|
||
#ifdef DJINNI_FUTURE_HAS_COROUTINE_SUPPORT | ||
|
||
namespace { | ||
|
||
template<typename Functor> | ||
struct OnCleanup { | ||
OnCleanup(Functor&& functor) | ||
:functor{std::move(functor)} | ||
{} | ||
~OnCleanup() { | ||
functor(); | ||
} | ||
Functor functor; | ||
}; | ||
|
||
djinni::Future<void> inner_coroutine(std::vector<int>& cleanup_ids, int id, djinni::Future<void> suspendOn) { | ||
OnCleanup cleanup{ | ||
[&] { | ||
cleanup_ids.push_back(id); | ||
} | ||
}; | ||
|
||
co_await suspendOn; | ||
co_return; // do not remove! | ||
} | ||
|
||
djinni::Future<void> outer_coroutine(std::vector<int>& cleanup_ids, int id, djinni::Future<void> suspendOn) { | ||
OnCleanup cleanup{ | ||
[&] { | ||
cleanup_ids.push_back(id); | ||
} | ||
}; | ||
|
||
co_await inner_coroutine(cleanup_ids, id + 1, std::move(suspendOn)); | ||
co_return; // do not remove! | ||
} | ||
|
||
} | ||
|
||
#endif | ||
|
||
@interface DBCppFutureTests : XCTestCase | ||
@end | ||
|
||
@implementation DBCppFutureTests | ||
|
||
#ifdef DJINNI_FUTURE_HAS_COROUTINE_SUPPORT | ||
- (void) testFutureCoroutines_cleanupOrder { | ||
std::vector<int> cleanupIds{}; | ||
|
||
djinni::Promise<void> djinniPromise{}; | ||
|
||
auto coroutineFuture = outer_coroutine(cleanupIds, 0, djinniPromise.getFuture()); | ||
XCTAssertFalse(coroutineFuture.isReady()); | ||
|
||
djinniPromise.setValue(); | ||
XCTAssertTrue(coroutineFuture.isReady()); | ||
|
||
XCTAssertEqual(cleanupIds.size(), 2); | ||
XCTAssertEqual(cleanupIds[0], 1); // the inner coroutine should be cleaned up first | ||
XCTAssertEqual(cleanupIds[1], 0); // ... then the outer | ||
} | ||
#endif | ||
|
||
@end |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find I have to change the feature detection to the following to make it work in c++17 + -fcoroutine-ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like with these settings the
<coroutine>
header is available but not functional. Your suggestion fixes that nicely, but I found another issue: with clang 16 and-std=c++20 -stdlib=libc++
, it chooses theexperimental
implementation even though the proper one is available.I've done some experiments:
__cpp_coroutines
is available for both C++20 and C++17 +-fcoroutines-ts
until clang 17 when the macro and-fcoroutines-ts
were completely removed__cpp_impl_coroutine
and__cpp_lib_coroutine
are only available in proper C++20 mode when we have<coroutine>
I've just pushed another suggestion based on these findings. Here's what I used to test it with a few compilers/settings: https://godbolt.org/z/rj737E5eq
Caveat: I've tested only with clang. For iOS and Android that seems to be the relevant compiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! this works really well.