Skip to content
Draft
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Library/src/Multiplayer/MultiplayerConnection.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,7 @@ namespace
Connection->SetDisconnected(
[&NetworkInterruptionCallback, &LogSystem](const std::exception_ptr& Except)
{
// We currently detect a connection interrupt if the disconnected callback contains an exception.
if (Except)
{
try
Expand All @@ -212,10 +213,9 @@ namespace
catch (const std::exception& e)
{
INVOKE_IF_NOT_NULL(NetworkInterruptionCallback, e.what());
LogSystem.LogMsg(csp::common::LogLevel::Log, "Connection Interrupted.");
Copy link
Contributor Author

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

}
}

LogSystem.LogMsg(csp::common::LogLevel::Log, "Connection Interrupted.");
});
}
}
Expand Down
80 changes: 80 additions & 0 deletions Tools/WrapperGenerator/Templates/C/GeneratedWrapper.mustache
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,86 @@

#pragma endregion Generated Includes

/*
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.
*/
#pragma region Emscripten Callback Queue

#if CSP_WASM
#include <emscripten.h>
#include <emscripten/proxying.h>
#include <emscripten/threading.h>
#include <pthread.h>
#include <tuple>

static em_proxying_queue* ProxyQueue = nullptr;
static pthread_t MainThread = 0;

// Creates an emscripten queue and sets the main thread if we haven't already initialized.
void Emscripten_InitializeProxyQueue()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Question] I don't understand why this logic is in a mustache file, and not in a proper .cpp with #EMSCRIPTEN guards like all of the rest of our emscripten specific code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. It would be a lot nicer in a cpp

{
if (ProxyQueue != nullptr)
{
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Question] Does this imply this Initialize method is called lazily? Is this a necessity, [consider] if it could be called during the standard initialize flow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct. I was worried about doing it when we first created the variable because its global. Maybe it's ok to do so.


ProxyQueue = em_proxying_queue_create();
MainThread = emscripten_main_runtime_thread_id();
}

// Structure to hold the callback and arguments.
template <typename ...T>
struct CallbackData
{
void (*Callback)(T...);
std::tuple<T...> Args;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Question] How does captured state play into this, seeing that this is a function pointer? Normally function pointer based callback approaches have a *UserContext sort of thing to work as a shim for that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Consider] Ah I see, the StateObject is always the first arg of the pack. Perhaps make that explicit and having the parameter pack only apply to user arguments in order to reduce arcanity, it's not possible to know how this works currently without also exploring the C-wrapper.

Copy link
Contributor

@MAG-ElliotMorris MAG-ElliotMorris Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Comment] This makes me worry about the "This will be trivial in embind" thing, we'll have to re-solve this. I suppose it's not the hugest thing.


// 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Comment] Pretty neat, I almost never see std::apply used. I think its only real purpose is to do this sort of parameter unpack.

Copy link
Contributor

@MAG-ElliotMorris MAG-ElliotMorris Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Nit] You might want to consider moving/forwarding the args, just for a bit of optimality for callbacks with value signatures. I struggle to trace the InData ownership however, so this might not be the thing to do.

}

// 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
{
Emscripten_InitializeProxyQueue();
// 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
#pragma endregion Emscripten Callback Queue
// 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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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. CallCallbackOnMainThread or something.

{
#ifdef CSP_WASM
Emscripten_CallbackOnThread(Callback, Args...);
#else
Callback(Args...);
#endif
}
extern "C"
{
typedef struct {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -239,12 +239,10 @@
{{/ type.is_string }}
{{/ parameters }}
{{ name }}(
{{ name }}_StateObject,
{{# parameters }}
CallCallback({{ name }}, {{ name }}_StateObject,
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 }}
Expand Down
4 changes: 1 addition & 3 deletions WasmTesting/csp-wasm.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can flip this test logic now its fixed!


});

Expand Down