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

Fix data race regarding server_quit in EditorExportPlatformWeb #88043

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

jsjtxietian
Copy link
Contributor

Fixes #87995

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good.

@RandomShaper Maybe we need some documentation for contributors on when/how to use SafeNumeric in threads.

@RandomShaper
Copy link
Member

Just for the records, if we had a time-outable semaphore wait function, this loop could be more similar to other ones we have in similar contexts across the codebase. As such, the exit flag could be a raw bool, as in the other cases.

Alternatively, if we had a Godotic relaxed atomic, it could have been used it, instead of SafeNumeric, which due to its acquire-release nature does more than is strictly needed here. The data race would be avoided just by ensuring the flag is read/written atomically, which generally would come for free at the assembly level.

All that said, and not being either the case, this PR does the right thing.

PS: Regarding docs, I hope to have some time at some point to write a few bullet points. Feel free to remind me in some time if I forget. 😃

@akien-mga akien-mga merged commit e898075 into godotengine:master Feb 7, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@jsjtxietian jsjtxietian deleted the web-data-race branch February 7, 2024 11:05
@Faless
Copy link
Collaborator

Faless commented Feb 7, 2024

I'm not sure this actually fixed the race condition.

The call to server->stop() might happen while the thread is inside ej->server->poll();. So AFAIU we should server_thread.wait_to_finish(); before calling server->stop().

@RandomShaper
Copy link
Member

The problem here was a data race: a thread reading from a memory location being written by another. That's not generically safe, so ensuring the reads and writes to that variable are atomic is enough.

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.

Data race in EditorExportPlatformWeb::~EditorExportPlatformWeb() detected by ThreadSanitizer
5 participants