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

Improve locking safety for RID handles builds #60427

Merged
merged 1 commit into from
Apr 25, 2022

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Apr 22, 2022

Additional locks are added. This is primarily to cover a potential race condition where the pool is resized from another thread during a get operation.

Notes

  • This is compiled out of regular builds (so has no effect in vanilla Godot)
  • It improves the thread safety of rids=handles and rids=tracked_handles builds.
  • Only noticed this as we were discussing the handles builds in random getornull error spammed in console #60406.
  • To my knowledge, these race condition haven't occurred for me in 5 months of use, but it is probably a good idea to close these possibilities.
  • Luckily it doesn't seem to have much effect on performance adding the extra lock on the get. Godot 4 puts this lock on RID gets as well, probably for similar reasons.

Note that by design in Godot 3.x, the RID objects themselves aren't thread safe. The RID database itself is intended to be thread safe, but there's nothing to stop one thread e.g. deleting a RID object mid way while another thread is accessing that object.

@lawnjelly lawnjelly requested a review from a team as a code owner April 22, 2022 10:56
@lawnjelly lawnjelly added this to the 3.5 milestone Apr 22, 2022
core/os/mutex.h Outdated Show resolved Hide resolved
@lawnjelly lawnjelly force-pushed the rid_handles_safer_locks branch 2 times, most recently from bab8cee to 6bc566b Compare April 23, 2022 08:07
Additional locks are added. This is primarily to cover a potential race condition where the pool is resized from another thread during a get operation.
@akien-mga akien-mga merged commit 9260dd3 into godotengine:3.x Apr 25, 2022
@akien-mga
Copy link
Member

Thanks!

@lawnjelly lawnjelly deleted the rid_handles_safer_locks branch April 26, 2022 07:30
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.

3 participants