Skip to content

[embind] Initialize embind on workers after main thread ctors. #24322

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

brendandahl
Copy link
Collaborator

@brendandahl brendandahl commented May 13, 2025

Previously, when a statically constructed thread was created it could potentially run before all of the embind static constructors on the main thread had run. This caused the workers to miss the embind bindings.

To fix this, I've added a std::atomic so that each worker waits for embind to be ready before running the binding code.

Fixes #19849

Previously, when a statically constructed thread was created it could
potentially run before all of the embind static constructors on the main
thread had run. This caused the workers to miss the embind bindings.

To fix this, I've added a `std::atomic` so that each worker waits for
embind to be ready before running the binding code.
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

lgtm % comment

#if PTHREADS || WASM_WORKERS
$postCtors: null, // Unused symbol stub for injecting post ctors call.
$postCtors__postset: () => { addAtPostCtor('__embind_post_ctors()'); },
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we just make this is __postset of some function that a core embind dependnecy? Adding this new symbol and forcing it to be included seems a more complex than it needs to be.

@kripken
Copy link
Member

kripken commented May 13, 2025

Did the priority option not work out? Either

  1. Adjust the embind constructor priority to be before all user constructors. You said this might not work since it needs user stuff to run in some cases...
  2. Document the issue and tell users to adjust the priority of their own constructors to be later. This seems safer to me than workers waiting on the main thread, which could deadlock IIUC?

@sbc100
Copy link
Collaborator

sbc100 commented May 14, 2025

Did the priority option not work out? Either

  1. Adjust the embind constructor priority to be before all user constructors. You said this might not work since it needs user stuff to run in some cases...

This doesn't work, at least last time we tried it it didn't work because there are (apparently) cases there the EMSCIRPTEN_BINDINGS block must be run after the static initializers that precede it in the same file.

Obviously we can look into those cases and see if we can change things around, but last time we tried to do this it got reverted.

  1. Document the issue and tell users to adjust the priority of their own constructors to be later. This seems safer to me than workers waiting on the main thread, which could deadlock IIUC?

This doesn't work because unfortunately the default for C++ constructors is the highest possible priority so its not possible to make things to run after default C++ constructors.

@sbc100
Copy link
Collaborator

sbc100 commented May 14, 2025

I think the possible deadlock risk here is very low because it would mean writing code that both creates a thread and expects to be up and running and interact-able-with all before main even runs. Doing that kind of thing in static constructors goes against most folks C/C++ sensibilities I think.

We could emit a warning in debug builds when thread end up waiting here?

@brendandahl
Copy link
Collaborator Author

Did the priority option not work out? Either

  1. Adjust the embind constructor priority to be before all user constructors. You said this might not work since it needs user stuff to run in some cases...

The following is supported in embind:

const std::string global_string = "global string";

EMSCRIPTEN_BINDINGS(constants) {
  emscripten::constant("global_string", global_string);
}

Which doesn't work if we run the binding code before user's static constructors. I think this is a relatively recent change (~2yrs), so we could potentially disallow it or tell users that they need to set the priority on the global_string to be higher than the embind code.

@kripken
Copy link
Member

kripken commented May 14, 2025

I see, thanks @sbc100 @brendandahl

This PR sgtm then. Is there a way we can improve debugging here? E.g. in a debug build, perhaps show a warning when we end up waiting for that lock?

@sbc100
Copy link
Collaborator

sbc100 commented May 14, 2025

I see, thanks @sbc100 @brendandahl

This PR sgtm then. Is there a way we can improve debugging here? E.g. in a debug build, perhaps show a warning when we end up waiting for that lock?

Yes that seem reasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

embind cannot be initialized correctly in a thread created in static constructor
3 participants