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

Calling MultiplayerAPI in a thread results in crashes. #79323

Closed
Leshy-YA opened this issue Jul 11, 2023 · 9 comments
Closed

Calling MultiplayerAPI in a thread results in crashes. #79323

Leshy-YA opened this issue Jul 11, 2023 · 9 comments

Comments

@Leshy-YA
Copy link

Godot version

4.1

System information

Godot v4.1.stable - Fedora Linux 38 (KDE Plasma) - Vulkan (Forward+) - integrated Intel(R) Xe Graphics (TGL GT2) () - 11th Gen Intel(R) Core(TM) i7-11370H @ 3.30GHz (8 Threads)

Issue description

Whenever a MultiplayerAPI call is made in a function running in a thread, that thread will crash.

Issues started occurring in 4.1 in all 4.0 releases this worked fine.

Steps to reproduce

Start a thread.
Call multiplayer.get_unique_id() in that function.
The function will exit immediately.

NOTE: No debug information will be provided due to: #18330

Minimal reproduction project

multiplayer_thread_test.zip

@AThousandShips
Copy link
Member

AThousandShips commented Jul 11, 2023

Multiplayer is only supported from the main thread, crash probably caused by manipulating the invalid result from get_multiplayer

@Leshy-YA
Copy link
Author

Multiplayer is only supported from the main thread (...)

I can't find any information in the docs of that being the case. Is that by design or just a long-standing issue?

@AThousandShips
Copy link
Member

By design, see the linked issue

APIs are non-thread safe unless stated otherwise, see here

Including this:

Interacting with the active scene tree is NOT thread-safe.

@AThousandShips
Copy link
Member

Should probably be more clearly documented though

@Leshy-YA
Copy link
Author

APIs are non-thread safe unless stated otherwise, see here

Oh ok! It would have been useful to have that also in the MultiplayerAPI doc.
Thanks for the info!

@AThousandShips
Copy link
Member

AThousandShips commented Jul 11, 2023

To clarify, the crash comes not from the thread safety as such, but from accessing methods on multiplayer when that is null, fixing the errors on threads, and documenting this should be the plan IMO

Not a regression as the change to enforce thread safety was intentional, see #75901

Will take a look at documenting this fact

Edit: Note that this crash would also happen if the node was not in the tree

@Chaosus
Copy link
Member

Chaosus commented Jun 2, 2024

Multiplayer is only supported from the main thread

IMO, even if it's not possible it shouldn't crash, instead an error should be printed to console/output.

@AThousandShips
Copy link
Member

AThousandShips commented Jun 2, 2024

The problem is that if you fetch the multiplayer and then call something on it how do you avoid a crash then? The returned value from multiplayer will be null

You'll get an error but still a crash

@AThousandShips
Copy link
Member

AThousandShips commented Jun 2, 2024

Calling things like is_multiplayer_authority used to crash but was fixed, but since it's up to the user to add null checks we can't fix user code that doesn't do that with multiplayer

I attempted to document this in the class reference but it wasn't approved, and didn't end up figuring out a good place to document it in the manual, but if someone could do that it'd be useful to add to it

Had forgotten that the decision was to close this after the PR was closed but didn't end up happening, so will close this and if improving documentation in the manual is desired it can be tracked over in godot-docs

But there are plenty of other methods that return null when called on a thread (including such fundamental methods as get_child) so not sure this is necessary, there is an error message printed if on the thread so it would be clear still

@AThousandShips AThousandShips removed this from the 4.x milestone Jun 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants