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

Fix race condition between handshake and waker registration #35

Merged
merged 1 commit into from
Jan 30, 2025

Conversation

mmastrac
Copy link
Contributor

@mmastrac mmastrac commented Jan 27, 2025

Ensure that wakers are registered in a thread-safe manner.

This is a latent multi-threaded bug that could happen, though it's a very, very tight race to win.

In a multi-threaded runtime, there was a very small window for a race:

  1. The send_handshake part of the handshake task completes, and the handshake then tries to wake any deferred wakers (which do not exist). The task is about to complete but then the thread gets parked by the CPU.
  2. In another thread, a poll_read operation (or similar) tests to see if the handshake finished, sees that it isn't and tries to register its waker. The handshake task has already tried to wake the deferred wakers, so nothing ever tries to wake them again.
  3. The handshake task completes, but no wakers are woken to complete the handshake process, locking up the connection.

This wasn't easy to repro on my Mac -- I had to explicitly add a thread sleep after the poll_read check for handle.is_finished(), but before we set the waker. This would trigger the race pretty much every time.

@mmastrac mmastrac mentioned this pull request Jan 27, 2025
@bartlomieju bartlomieju merged commit 415d580 into denoland:main Jan 30, 2025
6 checks passed
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