Skip to content

fix(vpn): use unbuffered channel in speaker #15863

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

Merged
merged 1 commit into from
Dec 16, 2024
Merged

Conversation

ethanndickson
Copy link
Member

@ethanndickson ethanndickson commented Dec 13, 2024

Closes coder/internal#253.

The Tunnel closing mechanism encounters a race if the passed io.ReadWriteCloser is shared between the Tunnel and the Manager, specifically if the closing of the Conn by one causes a read fail on the other.

The sequence of events during the flakey tests is as follows:

  1. Tunnel sends stop response
  2. Tunnel closes sendch
  3. net.Pipe gets closed by Tunnel's sendLoop ending
  4. Manager's serdes reads the response successfully and passes it to the speaker via the recvCh.
  5. The speaker tries to deliver the response via the message's respCh. The respCh is buffered so this is non-blocking.
  6. The recvLoopDone channel is closed by the recvCh getting closed (by failing to read from the closed conn)
  7. unaryRPC is finally scheduled to run, where both of these conditions in a select block are ready, and so whether or not the response is returned is random.
[...]
	case <-s.recvLoopDone:
		logger.Debug(s.ctx, "recvLoopDone while waiting for response")
		return resp, io.ErrUnexpectedEOF
	case resp = <-respCh:
		logger.Debug(s.ctx, "got response", slog.F("resp", resp))
		return resp, nil

The solution is to therefore make the respCh unbuffered. That way, it's not possible for the recvLoopDone channel to be closed until the respCh is read from.

Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@ethanndickson ethanndickson force-pushed the ethan/tunnel-flake branch 4 times, most recently from 5b19e5e to 7fb2642 Compare December 16, 2024 05:53
@ethanndickson ethanndickson changed the title fix: use independent pipes in vpn tunnel tests fix(vpn): use unbuffered channel in speaker Dec 16, 2024
@ethanndickson ethanndickson marked this pull request as ready for review December 16, 2024 06:03
@ethanndickson ethanndickson merged commit 67fdbe5 into main Dec 16, 2024
43 checks passed
@ethanndickson ethanndickson deleted the ethan/tunnel-flake branch December 16, 2024 08:48
@github-actions github-actions bot locked and limited conversation to collaborators Dec 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test flake: vpn TestTunnel_StartStop
2 participants