Skip to content

Commit

Permalink
Fix write lock deadline handling (#7)
Browse files Browse the repository at this point in the history
## 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.
  • Loading branch information
tsachiherman authored Nov 17, 2021
1 parent 19af735 commit 84c5e74
Showing 1 changed file with 34 additions and 1 deletion.
35 changes: 34 additions & 1 deletion conn.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,10 @@ var ErrCloseSent = errors.New("websocket: close sent")
// read limit set for the connection.
var ErrReadLimit = errors.New("websocket: read limit exceeded")

// ErrWriteDeadlineExceeded is returned when a writing to a connection has exceeded
// the specified deadline
var ErrWriteDeadlineExceeded = errors.New("websocket: writing deadline exceeded")

// netError satisfies the net Error interface.
type netError struct {
msg string
Expand Down Expand Up @@ -428,7 +432,36 @@ func (c *Conn) read(n int) ([]byte, error) {
}

func (c *Conn) write(frameType int, deadline time.Time, buf0, buf1 []byte) error {
<-c.mu
// if user has provided no deadline
if deadline.IsZero() {
// just wait for the mutex.
<-c.mu
} else {
// see how far the deadline is from now
sleepTime := deadline.Sub(time.Now())

// is the deadline pointing to the future, or the past ?
if sleepTime < 0 {
// operation timed out already.
return ErrWriteDeadlineExceeded
}

select {
case <-c.mu:
// good, we've got the writing lock.

// the default case would be taken if the above lock could not be aquired right away.
default:
// writing lock is currently taken.
select {
case <-c.mu:
// great ! we got the writing lock.
case <-time.After(sleepTime):
// we were not able to aquire the writing lock.
return ErrWriteDeadlineExceeded
}
}
}
defer func() { c.mu <- true }()

c.writeErrMu.Lock()
Expand Down

0 comments on commit 84c5e74

Please sign in to comment.