Skip to content
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

[Web] Fix dlopen threading issues by pre-opening all GDExtensions #92768

Closed

Conversation

Faless
Copy link
Collaborator

@Faless Faless commented Jun 4, 2024

Calling dlopen from a thread cause stalling on some browsers (notably firefox: #86382), the only valid workaround seems to be to dlopen all GDExtensions from the main thread as soon as possible.
We then keep track of the pre-opened libraries, and return the correct stored when a thread try to dlopen.

This means still no editor+thread+dlink support yet (since we don't know which libraries to open at startup in that case).

Draft status:

Calling dlopen from a thread cause stalling on some browsers, the only
valid workaround seems to be to dlopen all GDExtensions from the main
thread as soon as possible.
We then keep track of the pre-opened libraries, and return the correct
stored when a thread try to dlopen.

This means still no editor+thread+dlink support yet (since we don't know
which libraries to open at startup in that case).
@adamscott
Copy link
Member

Instead of using "fake" and "real" terminology, why not instead use "phase1" and "phase2"? Phase 1 loads phase 2, so we would have phase1_loop() (with a comment // Does nothing.), and a phase2_loop.

@AThousandShips AThousandShips added this to the 4.3 milestone Jun 5, 2024
Co-authored-by: Adam Scott <ascott.ca@gmail.com>
@dsnopek
Copy link
Contributor

dsnopek commented Jun 10, 2024

As a workaround, this seems fine.

It would be nice if we could make OS::open_dynamic_library() asynchronous, so that its work could be deferred to the main thread. Since it's internal and not exposed to users, that's theoretically possible, but since exposed APIs that depend on it (like GDExtensionManager::load_extension()) are synchronous, I don't think we could easily do that. I suppose we could add a LOAD_STATUS_DEFERRED along with a signal to GDExtensionManager? But that's not something we should really try to do for Godot 4.3.

@dsnopek
Copy link
Contributor

dsnopek commented Jun 14, 2024

Is this one superseded by PR #93143 now?

@adamscott
Copy link
Member

Is this one superseded by PR #93143 now?

By the look of it, yes, it seems so.

@Faless
Copy link
Collaborator Author

Faless commented Jun 14, 2024

Is this one superseded by PR #93143 now?

Yes it is, closing.

@Faless Faless closed this Jun 14, 2024
@Faless Faless removed this from the 4.3 milestone Jun 14, 2024
@Faless Faless deleted the web/fix_dlopen_by_preloading branch June 14, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants