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

During racy initialization, a race can happen where a notification is dropped #138

Closed
notgull opened this issue May 27, 2024 · 2 comments · Fixed by #139
Closed

During racy initialization, a race can happen where a notification is dropped #138

notgull opened this issue May 27, 2024 · 2 comments · Fixed by #139

Comments

@notgull
Copy link
Member

notgull commented May 27, 2024

The following logic exists in notify:

if let Some(inner) = self.try_inner() {

The intent is to make it so notifying an Event that has never has an EventListener is a single atomic operation and nothing else. However, a race condition can happen here where a notification is dropped even when there is a listener available.

  • Thread 0 calls listen(). As part of listen() the inner() function is called, which allocates the Arc<Inner> and compare_exchanges it into the inner pointer.
  • However, between the load of the inner pointer and the final compare_exchange, Thread 0 is pre-empted and Thread 1 starts.
  • Thread 1 calls notify, which then calls try_inner, which loads the inner pointer. Since it hasn't been updated yet this returns 0, which the notify function interprets as the event not listening yet.
  • Thread 1 finished notify by notifying no listeners.

If Thread 0 pre-empted Thread 1 while Thread 1 was updating some atomic state, this can lead to a deadlock.

@notgull
Copy link
Member Author

notgull commented May 27, 2024

It seems like this can also happen with the needs_notification optimization under loom's model.

notgull added a commit that referenced this issue May 27, 2024
The notify() function contains two optimizations:

- If the `inner` field is not yet initialized (i.e. no listeners have been
  created yet), it returns `0` from the function early.
- If the atomic `notified` field indicates that the current notification would
  do nothing, it returns `0` early as well.

`loom` testing has exposed race conditions in these optimizations that can cause
notifications to be missed, leading to deadlocks. This commit removes these
optimizations in the hope of preventing these deadlocks. Ideally we can restore
them in the future.

Closes #138
cc #130 and #133

Signed-off-by: John Nunley <[email protected]>
@notgull
Copy link
Member Author

notgull commented May 27, 2024

MIRI tests on async-lock pass with #139, so it looks like this is the underlying issue for #123

notgull added a commit that referenced this issue May 28, 2024
The notify() function contains two optimizations:

- If the `inner` field is not yet initialized (i.e. no listeners have been
  created yet), it returns `0` from the function early.
- If the atomic `notified` field indicates that the current notification would
  do nothing, it returns `0` early as well.

`loom` testing has exposed race conditions in these optimizations that can cause
notifications to be missed, leading to deadlocks. This commit removes these
optimizations in the hope of preventing these deadlocks. Ideally we can restore
them in the future.

Closes #138
cc #130 and #133

Signed-off-by: John Nunley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

1 participant