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

Deadlock workaround #219

Merged
merged 4 commits into from
May 30, 2024
Merged

Deadlock workaround #219

merged 4 commits into from
May 30, 2024

Conversation

mbrandonw
Copy link
Member

Right now we do a DispatchQueue.main.sync in order to make sure that the test observer is registered on the main thread. sync is always a dangerous operation, and it turns out we can deadlock. It can be reproduced with the following 3 steps:

  1. Start background thread/queue and access a dependency after a small amount of time.
  2. Before the dependency is accessed block the main thread with some work.
  3. After the main thread work is done access another dependency.

This causes a deadlock because the background thread has already entered into a dispatch_once to resolve static var current but is currently waiting on the main thread to finish its work. And simultaneously the main thread has now also entered the dispatch_once, and hence a deadlock.

The fix is perhaps a little risky, but we think it should be ok. We are going to allow adding the test observer in main.async if the dependency is first being accessed on a background thread. It seems theoretically possible that the test observer will not be registered by the time the test ends, but we haven't been able to find a situation in which that occurs.

@mbrandonw
Copy link
Member Author

Incidentally this will fix #76 too.

Copy link
Member

@stephencelis stephencelis 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!

@mbrandonw mbrandonw merged commit 76b98de into main May 30, 2024
7 checks passed
@mbrandonw mbrandonw deleted the deadlock-test branch May 30, 2024 01:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants