diff --git a/conn.go b/conn.go index f91b11b..eff26c6 100644 --- a/conn.go +++ b/conn.go @@ -214,6 +214,7 @@ type Conn struct { writeFrameType int // type of the current frame. writeSeq int // incremented to invalidate message writers. writeDeadline time.Time + isWriting bool // for best-effort concurrent write detection // Read fields readErr error @@ -227,6 +228,7 @@ type Conn struct { readMaskKey [4]byte handlePong func(string) error handlePing func(string) error + readErrCount int } 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) + if !c.isWriting { + panic("concurrent write to websocket connection") + } + c.isWriting = false + // Setup for next frame. c.writePos = maxFrameHeaderSize c.writeFrameType = continuationFrame @@ -734,7 +749,7 @@ func (c *Conn) advanceFrame() (int, error) { closeCode = int(binary.BigEndian.Uint16(payload)) 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} } @@ -752,9 +767,10 @@ func (c *Conn) handleProtocolError(message string) error { // There can be at most one open reader on a connection. NextReader discards // the previous message if the application has not already consumed it. // -// Errors returned from NextReader are permanent. If NextReader returns a -// non-nil error, then all subsequent calls to NextReader return the same -// error. +// Applications must break out of the application's read loop when this method +// returns a non-nil error value. Errors returned from this method are +// 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) { c.readSeq++ @@ -770,6 +786,15 @@ func (c *Conn) NextReader() (messageType int, r io.Reader, err error) { 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 } diff --git a/conn_test.go b/conn_test.go index 251254e..04c8dd8 100644 --- a/conn_test.go +++ b/conn_test.go @@ -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") +} diff --git a/doc.go b/doc.go index 609dde8..499b03d 100644 --- a/doc.go +++ b/doc.go @@ -85,14 +85,28 @@ // and pong. Call the connection WriteControl, WriteMessage or NextWriter // methods to send a control message to the peer. // -// Connections handle received ping and pong messages by invoking a callback -// function set with SetPingHandler and SetPongHandler methods. These callback -// functions can be invoked from the ReadMessage method, the NextReader method -// or from a call to the data message reader returned from NextReader. +// Connections handle received ping and pong messages by invoking callback +// functions set with SetPingHandler and SetPongHandler methods. The default +// ping handler sends a pong to the client. The callback functions can be +// invoked from the NextReader, ReadMessage or the message Read method. // -// Connections handle received close messages by returning an error from the -// ReadMessage method, the NextReader method or from a call to the data message -// reader returned from NextReader. +// Connections handle received close messages by sending a close message to the +// peer and returning a *CloseError from the the NextReader, ReadMessage or the +// 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 // @@ -107,22 +121,6 @@ // The Close and WriteControl methods can be called concurrently with all other // 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 // // Web browsers allow Javascript applications to open a WebSocket connection to