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.

Sign up for GitHub

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

Jump to bottom

Fix potential deadlock in AssetServer on single-threaded modes. #15808

Merged
merged 1 commit into from
Oct 11, 2024
Merged

Fix potential deadlock in AssetServer on single-threaded modes. #15808

merged 1 commit into from
Oct 11, 2024

Conversation

andriyDev
Copy link
Contributor

Objective

Fixes #15807

Solution

We move the guard into this function.

Testing

N/A, This is just reverting to the old behavior before #15509.

@andriyDev
Copy link
Contributor Author

Note an alternative is to do the dropping of the lock outside the function, but that would mean duplicating it twice.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Assets Load files from disk to use for things like images, models, and sounds S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Async Deals with asynchronous abstractions labels Oct 10, 2024
aecsocket
aecsocket approved these changes Oct 10, 2024
View reviewed changes
Copy link
Contributor

@aecsocket aecsocket left a comment

Choose a reason for hiding this comment

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

I think this change makes sense, I don't entirely like passing the RwLockWriteGuard across functions but that's fine if this logic is more correct.

JMS55
JMS55 approved these changes Oct 11, 2024
View reviewed changes
Copy link
Contributor

@JMS55 JMS55 left a comment

Choose a reason for hiding this comment

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

Didn't test, but seems fine to me.

@mockersf
Copy link
Member

does this fix also #15508?

@andriyDev
Copy link
Contributor Author

does this fix also #15508?

Yes, this also seems to fix #15508, at least the reproduction case provided (I tested it on HEAD first to make sure it was still broken). I don't know why @alice-i-cecile reopened the issue though, so maybe there's another cause?

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 11, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 11, 2024
Merged via the queue into bevyengine:main with commit 60a9a81 Oct 11, 2024
32 checks passed
@andriyDev andriyDev deleted the deadlock branch October 16, 2024 04:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aecsocket aecsocket aecsocket approved these changes

@JMS55 JMS55 JMS55 approved these changes

Assignees
No one assigned
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Bug An unexpected or incorrect behavior D-Async Deals with asynchronous abstractions S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Possible deadlock in AssetServer in single-threaded modes.
5 participants
@andriyDev @mockersf @aecsocket @JMS55 @alice-i-cecile