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

[4.3] Remove outer navigation map read locks #101535

Merged
merged 1 commit into from
Jan 30, 2025
Merged

Conversation

smix8
Copy link
Contributor

@smix8 smix8 commented Jan 14, 2025

Removes outer navigation map read locks as we apparently go where no one knows what will happen when we dare to lock twice.

Fixes #101318 for Godot 4.3.

Not relevant for Godot 4.2. as that was happy-thread-crash-land without those locks and also not relevant for Godot 4.4 as that has this entire code changed and works with a single map iteration lock.

Removes outer navigation map read locks.
@smix8 smix8 added bug topic:navigation cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Jan 14, 2025
@smix8 smix8 changed the title Remove outer navigation map read locks [4.3] Remove outer navigation map read locks Jan 14, 2025
@smix8 smix8 removed the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Jan 14, 2025
@akien-mga akien-mga added this to the 4.3 milestone Jan 14, 2025
@akien-mga
Copy link
Member

For the record, this was changed in 4.4 with d51615b.

There's a few more RWLockRead read_lock(map_rwlock); in the file, but I assume the fix here is to just avoiding calling it twice in functions which then call get_closest_point_info?

@smix8
Copy link
Contributor Author

smix8 commented Jan 14, 2025

Yes, the other locks need to stay, else we crash again with user pathfinding threads like we did in Godot 4.2 when the map changes and invalidates all the pointers. This only removes the outer function locks because they all call the internal get_closest_point_info() function that also read locks the same lock.

@akien-mga akien-mga merged commit 7f166be into godotengine:4.3 Jan 30, 2025
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@smix8 smix8 deleted the locknroll branch February 10, 2025 08:47
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.

2 participants