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

Prevent crash when accessing Node Multiplayer from thread #79332

Merged
merged 1 commit into from
Aug 1, 2023

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Jul 11, 2023

Prevents crashes when accessing a null-pointer on thread check failing in SceneTree, still won't prevent the same on access to multiplayer from script, where you still need to check for null

Didn't use ERR_FAIL_NULL because we already get an error from SceneTree so felt excessive to add another error message

Could alternatively add a check here for threading, but thought best to leave it in SceneTree to prevent confusion/divergence

Haven't tested since I don't have mono set up but might prevent a crash in #77707

@AThousandShips AThousandShips added topic:core crash topic:multiplayer cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Jul 11, 2023
@AThousandShips AThousandShips requested a review from a team July 11, 2023 11:10
@AThousandShips AThousandShips requested a review from a team as a code owner July 11, 2023 11:10
@akien-mga akien-mga added the bug label Jul 11, 2023
@akien-mga akien-mga added this to the 4.2 milestone Jul 11, 2023
@Faless
Copy link
Collaborator

Faless commented Aug 1, 2023

I'm not super up to date regarding threading macros, but I think we could use:

ERR_READ_THREAD_GUARD_V(Ref<MultiplayerAPI>())

and

ERR_READ_THREAD_GUARD_V(ERR_LOCKED)

cc @RandomShaper

@AThousandShips
Copy link
Member Author

AThousandShips commented Aug 1, 2023

My reasoning here was to leave the error messages to the SceneTree side to avoid duplication and match changes over there, but can easily add checks directly here, though it'll mean checks run twice

@Faless
Copy link
Collaborator

Faless commented Aug 1, 2023

Ah, that makes sense!

Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

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

LGTM 👍 🏆

@RandomShaper
Copy link
Member

I think the reasoning about the checks makes sense.

@YuriSizov YuriSizov merged commit fd1ee5d into godotengine:master Aug 1, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@AThousandShips AThousandShips deleted the mp_crash branch August 1, 2023 15:56
@AThousandShips
Copy link
Member Author

Thank you!

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.2.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants