Address common gotchas with package

Update documentation to explicitly state that applications must break out of a
read loop on error.

Detect application read loops spinning on a failed connection and panic.

Detect concurrent writes and panic. The detection is best-effort.

Update documentation to state that connections respond to close frames.
This commit is contained in:
Gary Burd 2016-02-16 15:03:54 -08:00
parent e85d897656
commit 0e2713e645
3 changed files with 105 additions and 28 deletions

35
conn.go
View File

@ -214,6 +214,7 @@ type Conn struct {
writeFrameType int // type of the current frame. writeFrameType int // type of the current frame.
writeSeq int // incremented to invalidate message writers. writeSeq int // incremented to invalidate message writers.
writeDeadline time.Time writeDeadline time.Time
isWriting bool // for best-effort concurrent write detection
// Read fields // Read fields
readErr error readErr error
@ -227,6 +228,7 @@ type Conn struct {
readMaskKey [4]byte readMaskKey [4]byte
handlePong func(string) error handlePong func(string) error
handlePing func(string) error handlePing func(string) error
readErrCount int
} }
func newConn(conn net.Conn, isServer bool, readBufferSize, writeBufferSize int) *Conn { func newConn(conn net.Conn, isServer bool, readBufferSize, writeBufferSize int) *Conn {
@ -440,9 +442,22 @@ func (c *Conn) flushFrame(final bool, extra []byte) error {
} }
} }
// Write the buffers to the connection. // Write the buffers to the connection with best-effort detection of
// concurrent writes. See the concurrency section in the package
// documentation for more info.
if c.isWriting {
panic("concurrent write to websocket connection")
}
c.isWriting = true
c.writeErr = c.write(c.writeFrameType, c.writeDeadline, c.writeBuf[framePos:c.writePos], extra) c.writeErr = c.write(c.writeFrameType, c.writeDeadline, c.writeBuf[framePos:c.writePos], extra)
if !c.isWriting {
panic("concurrent write to websocket connection")
}
c.isWriting = false
// Setup for next frame. // Setup for next frame.
c.writePos = maxFrameHeaderSize c.writePos = maxFrameHeaderSize
c.writeFrameType = continuationFrame c.writeFrameType = continuationFrame
@ -734,7 +749,7 @@ func (c *Conn) advanceFrame() (int, error) {
closeCode = int(binary.BigEndian.Uint16(payload)) closeCode = int(binary.BigEndian.Uint16(payload))
closeText = string(payload[2:]) closeText = string(payload[2:])
} }
c.WriteControl(CloseMessage, echoMessage, time.Now().Add(writeWait)) c.WriteControl(CloseMessage, echoMessage, time.Now().Add(writeWait))
return noFrame, &CloseError{Code: closeCode, Text: closeText} return noFrame, &CloseError{Code: closeCode, Text: closeText}
} }
@ -752,9 +767,10 @@ func (c *Conn) handleProtocolError(message string) error {
// There can be at most one open reader on a connection. NextReader discards // There can be at most one open reader on a connection. NextReader discards
// the previous message if the application has not already consumed it. // the previous message if the application has not already consumed it.
// //
// Errors returned from NextReader are permanent. If NextReader returns a // Applications must break out of the application's read loop when this method
// non-nil error, then all subsequent calls to NextReader return the same // returns a non-nil error value. Errors returned from this method are
// error. // permanent. Once this method returns a non-nil error, all subsequent calls to
// this method return the same error.
func (c *Conn) NextReader() (messageType int, r io.Reader, err error) { func (c *Conn) NextReader() (messageType int, r io.Reader, err error) {
c.readSeq++ c.readSeq++
@ -770,6 +786,15 @@ func (c *Conn) NextReader() (messageType int, r io.Reader, err error) {
return frameType, messageReader{c, c.readSeq}, nil return frameType, messageReader{c, c.readSeq}, nil
} }
} }
// Applications that do handle the error returned from this method spin in
// tight loop on connection failure. To help application developers detect
// this error, panic on repeated reads to the failed connection.
c.readErrCount++
if c.readErrCount >= 1000 {
panic("repeated read on failed websocket connection")
}
return noFrame, nil, c.readErr return noFrame, nil, c.readErr
} }

View File

@ -311,3 +311,57 @@ func TestUnexpectedCloseErrors(t *testing.T) {
} }
} }
} }
type blockingWriter struct {
c1, c2 chan struct{}
}
func (w blockingWriter) Write(p []byte) (int, error) {
// Allow main to continue
close(w.c1)
// Wait for panic in main
<-w.c2
return len(p), nil
}
func TestConcurrentWritePanic(t *testing.T) {
w := blockingWriter{make(chan struct{}), make(chan struct{})}
c := newConn(fakeNetConn{Reader: nil, Writer: w}, false, 1024, 1024)
go func() {
c.WriteMessage(TextMessage, []byte{})
}()
// wait for goroutine to block in write.
<-w.c1
defer func() {
close(w.c2)
if v := recover(); v != nil {
return
}
}()
c.WriteMessage(TextMessage, []byte{})
t.Fatal("should not get here")
}
type failingReader struct{}
func (r failingReader) Read(p []byte) (int, error) {
return 0, io.EOF
}
func TestFailedConnectionReadPanic(t *testing.T) {
c := newConn(fakeNetConn{Reader: failingReader{}, Writer: nil}, false, 1024, 1024)
defer func() {
if v := recover(); v != nil {
return
}
}()
for i := 0; i < 20000; i++ {
c.ReadMessage()
}
t.Fatal("should not get here")
}

44
doc.go
View File

@ -85,14 +85,28 @@
// and pong. Call the connection WriteControl, WriteMessage or NextWriter // and pong. Call the connection WriteControl, WriteMessage or NextWriter
// methods to send a control message to the peer. // methods to send a control message to the peer.
// //
// Connections handle received ping and pong messages by invoking a callback // Connections handle received ping and pong messages by invoking callback
// function set with SetPingHandler and SetPongHandler methods. These callback // functions set with SetPingHandler and SetPongHandler methods. The default
// functions can be invoked from the ReadMessage method, the NextReader method // ping handler sends a pong to the client. The callback functions can be
// or from a call to the data message reader returned from NextReader. // invoked from the NextReader, ReadMessage or the message Read method.
// //
// Connections handle received close messages by returning an error from the // Connections handle received close messages by sending a close message to the
// ReadMessage method, the NextReader method or from a call to the data message // peer and returning a *CloseError from the the NextReader, ReadMessage or the
// reader returned from NextReader. // message Read method.
//
// The application must read the connection to process ping and close messages
// sent from the peer. If the application is not otherwise interested in
// messages from the peer, then the application should start a goroutine to
// read and discard messages from the peer. A simple example is:
//
// func readLoop(c *websocket.Conn) {
// for {
// if _, _, err := c.NextReader(); err != nil {
// c.Close()
// break
// }
// }
// }
// //
// Concurrency // Concurrency
// //
@ -107,22 +121,6 @@
// The Close and WriteControl methods can be called concurrently with all other // The Close and WriteControl methods can be called concurrently with all other
// methods. // methods.
// //
// Read is Required
//
// The application must read the connection to process ping and close messages
// sent from the peer. If the application is not otherwise interested in
// messages from the peer, then the application should start a goroutine to read
// and discard messages from the peer. A simple example is:
//
// func readLoop(c *websocket.Conn) {
// for {
// if _, _, err := c.NextReader(); err != nil {
// c.Close()
// break
// }
// }
// }
//
// Origin Considerations // Origin Considerations
// //
// Web browsers allow Javascript applications to open a WebSocket connection to // Web browsers allow Javascript applications to open a WebSocket connection to