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

Fixes shutdown deadlock in websocket client #1221

Merged
merged 3 commits into from
Jul 26, 2024

Conversation

fhats-stripe
Copy link
Contributor

Reviewers

r? @stripe/developer-products

Summary

This change solves a deadlock that can happen in stripe listen when the underlying websocket connection closes or is reconnecting while the websocket client is attempting to send a message. The deadlock happens when the writePump has a message to send:

	for {
		select {
		case outMsg, ok := <-c.send:

runs into an error trying to send the message because the connection has been closed for one reason or another:

			err = c.conn.WriteJSON(outMsg)
			if err != nil {

and attempts to re-queue the message it was going to process:

				// Requeue the message to be processed when writePump restarts
				c.send <- outMsg

c.send is unbuffered, and only read from in this select. Sending messages to c.send in the same block that processes reads from c.send will always deadlock, as message sending to unbuffered channels blocks until the message is read.

This PR fixes that deadlock by making c.send a buffered channel, allowing the re-queueing behavior we're looking for.

This condition is not all that common, but given stripe listen tries to reconnect every once in a while this could happen more often than you'd think in a busy stripe listen session.

This change comes with a unit test meant to provoke the regression to verify it remains fixed. That test was a little challenging to write (I'm still learning how to do these well), and to support the new test case I added some synchronization primitives in the websocket client to make it easier to monkey with the client from the test case.

@fhats-stripe fhats-stripe requested a review from a team as a code owner July 25, 2024 22:21
@fhats-stripe fhats-stripe changed the title Fhats/fix write pump shutdown deadlock Fixes shutdown deadlock in websocket client Jul 25, 2024
@charliecruzan-stripe charliecruzan-stripe self-assigned this Jul 25, 2024
Copy link
Contributor

@charliecruzan-stripe charliecruzan-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thankyou for the solid description, it makes this much easier to review!

@fhats-stripe fhats-stripe merged commit 7a252a1 into master Jul 26, 2024
19 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