-
Notifications
You must be signed in to change notification settings - Fork 14
[OF-1524] Cross-thread Callbacks #806
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
base: main
Are you sure you want to change the base?
Changes from all commits
f82f692
e245e36
83e4aee
fa27cbd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| /* | ||
| Callback queue designed to push callbacks onto the main thread using the emscripten api. | ||
| This works because emscriptens threads are just web workers, which allows us to push messages to different workers. | ||
|
|
||
| This is implemented to work around the limitation of callbacks needing to fire on the same thread they were created on. | ||
| */ | ||
|
|
||
| #ifdef CSP_WASM | ||
|
|
||
| #include <emscripten.h> | ||
| #include <emscripten/proxying.h> | ||
| #include <emscripten/threading.h> | ||
| #include <pthread.h> | ||
| #include <tuple> | ||
|
|
||
| namespace csp | ||
| { | ||
| static em_proxying_queue* ProxyQueue = em_proxying_queue_create(); | ||
| static pthread_t MainThread = emscripten_main_runtime_thread_id(); | ||
|
|
||
| // Structure to hold the callback and arguments. | ||
| // With the current implementation, the first argument will always be the callback context. | ||
| template <typename... T> struct CallbackData | ||
| { | ||
| void (*Callback)(T...); | ||
| std::tuple<T...> Args; | ||
| }; | ||
|
|
||
| // This function is called internally from emscripten_proxy_sync on the main thread. | ||
| template <typename... T> static void Emscripten_CallbackWrapper(void* InData) | ||
| { | ||
| auto* Data = static_cast<CallbackData<T...>*>(InData); | ||
| std::apply(Data->Callback, Data->Args); | ||
| } | ||
|
|
||
| // Function that will push the callback to the main thread if we are not on it. | ||
| template <typename... T> static void Emscripten_CallbackOnThread(void (*Callback)(T...), T... Args) | ||
| { | ||
| bool OnMainThread = pthread_equal(pthread_self(), emscripten_main_runtime_thread_id()); | ||
| if (OnMainThread) | ||
| { | ||
| // We're on the main thread already, just call normally | ||
| Callback(Args...); | ||
| } | ||
| else | ||
| { | ||
| // Pack our data and send to the emscripten queue. | ||
| CallbackData<T...> Data { Callback, std::make_tuple(Args...) }; | ||
| emscripten_proxy_sync(ProxyQueue, MainThread, Emscripten_CallbackWrapper<T...>, static_cast<void*>(&Data)); | ||
| } | ||
| } | ||
| } | ||
| #endif |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,20 @@ | |
|
|
||
| #pragma endregion Generated Includes | ||
|
|
||
| #include "EmscriptenBindings/CallbackQueue.h" | ||
|
|
||
| // Generic callback function which will either call the callback directly, or, | ||
| // if we're on wasm, push to the main thread if we're not on the main thread. | ||
| template <typename ...T> | ||
| static void CallCallback(void (*Callback)(T...), T... Args) | ||
|
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. [Consider] Naming this more specifically so in the C-api it's known why it exists. |
||
| { | ||
| #ifdef CSP_WASM | ||
| csp::Emscripten_CallbackOnThread(Callback, Args...); | ||
| #else | ||
| Callback(Args...); | ||
| #endif | ||
| } | ||
|
|
||
| extern "C" | ||
| { | ||
| typedef struct { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -239,12 +239,10 @@ | |
| {{/ type.is_string }} | ||
| {{/ parameters }} | ||
| {{ name }}( | ||
| {{ name }}_StateObject, | ||
| {{# parameters }} | ||
| CallCallback({{ name }}, {{ name }}_StateObject, | ||
|
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. Now calls the wrapper function instead |
||
| {{# parameters }} | ||
| {{# type.is_string }} | ||
| _{{ name }} | ||
| (const char*)_{{ name }} | ||
|
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. I was getting compile errors for strings, as were were creating chars. but we are expected cont char |
||
| {{/ type.is_string }} | ||
| {{^ type.is_string }} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,9 +50,7 @@ test('Cross Thread Callbacks From Log Callback, OB-3782', async () => { | |
| console.log(errors); | ||
|
|
||
| assert.ok( | ||
| errors.some(e => e.message.includes('table index is out of bounds')), | ||
| 'Expected cross-thread `table index is out of bounds` error. Message not found, did you fix the bug (OB-3782)? Nice job! ', | ||
| ); | ||
| !errors.some(e => e.message.includes('table index is out of bounds'))); | ||
|
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. We can flip this test logic now its fixed! |
||
|
|
||
| }); | ||
|
|
||
|
|
||
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.
Log should be in here, as this is when we call the NetworkInterruptionCallback