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 write lock deadline handling #7

Merged
merged 4 commits into from
Nov 17, 2021
Merged

Conversation

tsachiherman
Copy link

Issue

The existing code in go-algorand is using two go-routines for reading/writing and a separate go-routine for opening/closing the connections.
When a client connection becomes non-responsive ( i.e. the writer is not returning ), the current implementation tries to close the connection. However, per WebSocket specification, the client is supposed to send a Close message first.

The close message is being sent using the WriteControl method, which is supposed to be able to run concurrently with WriteMessage. However, both reaches Conn.write where it attempt to acquire a writing lock.

Since the WriteMessage -> Conn.write is stuck ( but the WriteControl doesn't really know it's stuck indefinitely ), the WriteControl -> Conn.write get blocks indefinitely as well.

Unlike the WriteMessage, when calling WriteControl, we provide a deadline : a timestamp after which we don't care if the message was actually sent. In this PR, I have added handling that would limit the time the Conn.write would wait to that deadline.

From spec perspective, it would be a violation of the WebSocket protocol; however, given that the other party is no longer responsive, a successful writing of a Close message is not possible, it would probably be just fine.

@tsachiherman tsachiherman self-assigned this Nov 17, 2021
@cce
Copy link

cce commented Nov 17, 2021

Related: gorilla#704

conn.go Outdated Show resolved Hide resolved
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