Skip to content

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

Merged
alice-i-cecile merged 1 commit intobevyengine:mainfrom
andriyDev:deadlock
Oct 11, 2024
Merged

Fix potential deadlock in AssetServer on single-threaded modes.#15808
alice-i-cecile merged 1 commit intobevyengine:mainfrom
andriyDev:deadlock

Conversation

@andriyDev
Copy link
Copy Markdown
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
Copy Markdown
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
Copy link
Copy Markdown
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.

Copy link
Copy Markdown
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
Copy Markdown
Member

does this fix also #15508?

@andriyDev
Copy link
Copy Markdown
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
@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

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

Development

Successfully merging this pull request may close these issues.

Possible deadlock in AssetServer in single-threaded modes.

5 participants