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 websocket reconnect race condition #7815

Merged
merged 18 commits into from May 13, 2023
Merged

Conversation

yschimke
Copy link
Collaborator

@yschimke yschimke commented May 11, 2023

Attempting a fix for #7381

The reason why it happens when fast reconnecting is that when we fail because of a 401, we release the Connection back to the ConnectionPool. The new connection started from the onFailure callback is very likely to get the same connection, and so two threads are now operating on the same socket.

    call!!.enqueue(object : Callback {
      override fun onResponse(call: Call, response: Response) {
        val exchange = response.exchange
        val streams: Streams
        try {
          checkUpgradeSuccess(response, exchange)
          streams = exchange!!.newWebSocketStreams()
        } catch (e: IOException) {
          exchange?.webSocketUpgradeFailed() // releases the connection back to the ConnectionPool
          failWebSocket(e, response)
          response.closeQuietly() // Drains the Response data
          return
        }

@yschimke yschimke marked this pull request as ready for review May 12, 2023 19:30
@yschimke yschimke requested a review from swankjesse May 12, 2023 19:30
@yschimke
Copy link
Collaborator Author

@swankjesse requesting post review, would like to test with a published snapshot.

From slack "fix looks good"

@yschimke yschimke merged commit f62fd47 into square:master May 13, 2023
19 of 20 checks passed
yschimke added a commit to yschimke/okhttp that referenced this pull request May 13, 2023
@yschimke
Copy link
Collaborator Author

Damn, I took a success as a pass but it was really a lucky run of a flaky test

swankjesse pushed a commit that referenced this pull request Sep 15, 2023
* Fix websocket reconnect race condition (#7815)

(cherry picked from commit f62fd47)

* Attempt to fix a flaky test

* Evict connection pool a second time after tests (#7819)

(cherry picked from commit e2344c7)

* Simplify
@yschimke yschimke deleted the fix_websocket branch October 15, 2023 11:54
Copy link
Member

@swankjesse swankjesse left a comment

Choose a reason for hiding this comment

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

LGTM

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.

None yet

2 participants