Skip to content

Commit

Permalink
Handle errcheck warnings
Browse files Browse the repository at this point in the history
The package ignored errors from net.Conn Set*Deadline in a few places.
Update the package to return these errors to the caller.

Ignore all other errors reported by errcheck. These errors are safe to
ignore because
- The function is making a best effort to cleanup while handling another
  error.
- The function call is guaranteed to succeed.
- The error is ignored in a test.
  • Loading branch information
Canelo Hill authored and jaitaiwan committed Jul 6, 2024
1 parent a62d9d2 commit 3810b23
Show file tree
Hide file tree
Showing 10 changed files with 99 additions and 51 deletions.
18 changes: 15 additions & 3 deletions client.go
Original file line number Diff line number Diff line change
Expand Up @@ -305,9 +305,15 @@ func (d *Dialer) DialContext(ctx context.Context, urlStr string, requestHeader h
})
}

// Close the network connection when returning an error. The variable
// netConn is set to nil before the success return at the end of the
// function.
defer func() {
if netConn != nil {
netConn.Close()
// It's safe to ignore the error from Close() because this code is
// only executed when returning a more important error to the
// application.
_ = netConn.Close()
}
}()

Expand Down Expand Up @@ -398,8 +404,14 @@ func (d *Dialer) DialContext(ctx context.Context, urlStr string, requestHeader h
resp.Body = io.NopCloser(bytes.NewReader([]byte{}))
conn.subprotocol = resp.Header.Get("Sec-Websocket-Protocol")

netConn.SetDeadline(time.Time{})
netConn = nil // to avoid close in defer.
if err := netConn.SetDeadline(time.Time{}); err != nil {
return nil, resp, err
}

// Success! Set netConn to nil to stop the deferred function above from
// closing the network connection.
netConn = nil

return conn, resp, nil
}

Expand Down
8 changes: 4 additions & 4 deletions client_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,7 @@ func TestRespOnBadHandshake(t *testing.T) {

s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(expectedStatus)
io.WriteString(w, expectedBody)
_, _ = io.WriteString(w, expectedBody)
}))
defer s.Close()

Expand Down Expand Up @@ -828,7 +828,7 @@ func TestSocksProxyDial(t *testing.T) {
}
defer c1.Close()

c1.SetDeadline(time.Now().Add(30 * time.Second))
_ = c1.SetDeadline(time.Now().Add(30 * time.Second))

buf := make([]byte, 32)
if _, err := io.ReadFull(c1, buf[:3]); err != nil {
Expand Down Expand Up @@ -867,10 +867,10 @@ func TestSocksProxyDial(t *testing.T) {
defer c2.Close()
done := make(chan struct{})
go func() {
io.Copy(c1, c2)
_, _ = io.Copy(c1, c2)
close(done)
}()
io.Copy(c2, c1)
_, _ = io.Copy(c2, c1)
<-done
}()

Expand Down
6 changes: 5 additions & 1 deletion compression.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,11 @@ func decompressNoContextTakeover(r io.Reader) io.ReadCloser {
"\x01\x00\x00\xff\xff"

fr, _ := flateReaderPool.Get().(io.ReadCloser)
fr.(flate.Resetter).Reset(io.MultiReader(r, strings.NewReader(tail)), nil)
mr := io.MultiReader(r, strings.NewReader(tail))
if err := fr.(flate.Resetter).Reset(mr, nil); err != nil {
// Reset never fails, but handle error in case that changes.
fr = flate.NewReader(mr)
}
return &flateReadWrapper{fr}
}

Expand Down
6 changes: 3 additions & 3 deletions compression_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ func TestTruncWriter(t *testing.T) {
if m > n {
m = n
}
w.Write(p[:m])
_, _ = w.Write(p[:m])
p = p[m:]
}
if b.String() != data[:len(data)-len(w.p)] {
Expand All @@ -46,7 +46,7 @@ func BenchmarkWriteNoCompression(b *testing.B) {
messages := textMessages(100)
b.ResetTimer()
for i := 0; i < b.N; i++ {
c.WriteMessage(TextMessage, messages[i%len(messages)])
_ = c.WriteMessage(TextMessage, messages[i%len(messages)])
}
b.ReportAllocs()
}
Expand All @@ -59,7 +59,7 @@ func BenchmarkWriteWithCompression(b *testing.B) {
c.newCompressionWriter = compressNoContextTakeover
b.ResetTimer()
for i := 0; i < b.N; i++ {
c.WriteMessage(TextMessage, messages[i%len(messages)])
_ = c.WriteMessage(TextMessage, messages[i%len(messages)])
}
b.ReportAllocs()
}
Expand Down
36 changes: 22 additions & 14 deletions conn.go
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,9 @@ func (c *Conn) read(n int) ([]byte, error) {
if err == io.EOF {
err = errUnexpectedEOF
}
c.br.Discard(len(p))
// Discard is guaranteed to succeed because the number of bytes to discard
// is less than or equal to the number of bytes buffered.
_, _ = c.br.Discard(len(p))
return p, err
}

Expand All @@ -385,7 +387,9 @@ func (c *Conn) write(frameType int, deadline time.Time, buf0, buf1 []byte) error
return err
}

c.conn.SetWriteDeadline(deadline)
if err := c.conn.SetWriteDeadline(deadline); err != nil {
return c.writeFatal(err)
}
if len(buf1) == 0 {
_, err = c.conn.Write(buf0)
} else {
Expand All @@ -395,7 +399,7 @@ func (c *Conn) write(frameType int, deadline time.Time, buf0, buf1 []byte) error
return c.writeFatal(err)
}
if frameType == CloseMessage {
c.writeFatal(ErrCloseSent)
_ = c.writeFatal(ErrCloseSent)
}
return nil
}
Expand Down Expand Up @@ -458,13 +462,14 @@ func (c *Conn) WriteControl(messageType int, data []byte, deadline time.Time) er
return err
}

c.conn.SetWriteDeadline(deadline)
_, err = c.conn.Write(buf)
if err != nil {
if err := c.conn.SetWriteDeadline(deadline); err != nil {
return c.writeFatal(err)
}
if _, err = c.conn.Write(buf); err != nil {
return c.writeFatal(err)
}
if messageType == CloseMessage {
c.writeFatal(ErrCloseSent)
_ = c.writeFatal(ErrCloseSent)
}
return err
}
Expand Down Expand Up @@ -628,7 +633,7 @@ func (w *messageWriter) flushFrame(final bool, extra []byte) error {
}

if final {
w.endMessage(errWriteClosed)
_ = w.endMessage(errWriteClosed)
return nil
}

Expand Down Expand Up @@ -815,7 +820,7 @@ func (c *Conn) advanceFrame() (int, error) {
rsv2 := p[0]&rsv2Bit != 0
rsv3 := p[0]&rsv3Bit != 0
mask := p[1]&maskBit != 0
c.setReadRemaining(int64(p[1] & 0x7f))
_ = c.setReadRemaining(int64(p[1] & 0x7f)) // will not fail because argument is >= 0

c.readDecompress = false
if rsv1 {
Expand Down Expand Up @@ -920,7 +925,8 @@ func (c *Conn) advanceFrame() (int, error) {
}

if c.readLimit > 0 && c.readLength > c.readLimit {
c.WriteControl(CloseMessage, FormatCloseMessage(CloseMessageTooBig, ""), time.Now().Add(writeWait))
// Make a best effort to send a close message describing the problem.
_ = c.WriteControl(CloseMessage, FormatCloseMessage(CloseMessageTooBig, ""), time.Now().Add(writeWait))
return noFrame, ErrReadLimit
}

Expand All @@ -932,7 +938,7 @@ func (c *Conn) advanceFrame() (int, error) {
var payload []byte
if c.readRemaining > 0 {
payload, err = c.read(int(c.readRemaining))
c.setReadRemaining(0)
_ = c.setReadRemaining(0) // will not fail because argument is >= 0
if err != nil {
return noFrame, err
}
Expand Down Expand Up @@ -979,7 +985,8 @@ func (c *Conn) handleProtocolError(message string) error {
if len(data) > maxControlFramePayloadSize {
data = data[:maxControlFramePayloadSize]
}
c.WriteControl(CloseMessage, data, time.Now().Add(writeWait))
// Make a best effor to send a close message describing the problem.
_ = c.WriteControl(CloseMessage, data, time.Now().Add(writeWait))
return errors.New("websocket: " + message)
}

Expand Down Expand Up @@ -1052,7 +1059,7 @@ func (r *messageReader) Read(b []byte) (int, error) {
}
rem := c.readRemaining
rem -= int64(n)
c.setReadRemaining(rem)
_ = c.setReadRemaining(rem) // rem is guaranteed to be >= 0
if c.readRemaining > 0 && c.readErr == io.EOF {
c.readErr = errUnexpectedEOF
}
Expand Down Expand Up @@ -1134,7 +1141,8 @@ func (c *Conn) SetCloseHandler(h func(code int, text string) error) {
if h == nil {
h = func(code int, text string) error {
message := FormatCloseMessage(code, "")
c.WriteControl(CloseMessage, message, time.Now().Add(writeWait))
// Make a best effor to send the close message.
_ = c.WriteControl(CloseMessage, message, time.Now().Add(writeWait))
return nil
}
}
Expand Down
4 changes: 2 additions & 2 deletions conn_broadcast_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,9 @@ func (b *broadcastBench) makeConns(numConns int) {
select {
case msg := <-c.msgCh:
if msg.prepared != nil {
c.conn.WritePreparedMessage(msg.prepared)
_ = c.conn.WritePreparedMessage(msg.prepared)
} else {
c.conn.WriteMessage(TextMessage, msg.payload)
_ = c.conn.WriteMessage(TextMessage, msg.payload)
}
val := atomic.AddInt32(&b.count, 1)
if val%int32(numConns) == 0 {
Expand Down
32 changes: 16 additions & 16 deletions conn_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ func TestControl(t *testing.T) {
wc := newTestConn(nil, &connBuf, isServer)
rc := newTestConn(&connBuf, nil, !isServer)
if isWriteControl {
wc.WriteControl(PongMessage, []byte(message), time.Now().Add(time.Second))
_ = wc.WriteControl(PongMessage, []byte(message), time.Now().Add(time.Second))
} else {
w, err := wc.NextWriter(PongMessage)
if err != nil {
Expand All @@ -174,7 +174,7 @@ func TestControl(t *testing.T) {
}
var actualMessage string
rc.SetPongHandler(func(s string) error { actualMessage = s; return nil })
rc.NextReader()
_, _, _ = rc.NextReader()
if actualMessage != message {
t.Errorf("%s: pong=%q, want %q", name, actualMessage, message)
continue
Expand Down Expand Up @@ -358,8 +358,8 @@ func TestCloseFrameBeforeFinalMessageFrame(t *testing.T) {
rc := newTestConn(&b1, &b2, true)

w, _ := wc.NextWriter(BinaryMessage)
w.Write(make([]byte, bufSize+bufSize/2))
wc.WriteControl(CloseMessage, FormatCloseMessage(expectedErr.Code, expectedErr.Text), time.Now().Add(10*time.Second))
_, _ = w.Write(make([]byte, bufSize+bufSize/2))
_ = wc.WriteControl(CloseMessage, FormatCloseMessage(expectedErr.Code, expectedErr.Text), time.Now().Add(10*time.Second))
w.Close()

op, r, err := rc.NextReader()
Expand All @@ -385,7 +385,7 @@ func TestEOFWithinFrame(t *testing.T) {
rc := newTestConn(&b, nil, true)

w, _ := wc.NextWriter(BinaryMessage)
w.Write(make([]byte, bufSize))
_, _ = w.Write(make([]byte, bufSize))
w.Close()

if n >= b.Len() {
Expand Down Expand Up @@ -419,7 +419,7 @@ func TestEOFBeforeFinalFrame(t *testing.T) {
rc := newTestConn(&b1, &b2, true)

w, _ := wc.NextWriter(BinaryMessage)
w.Write(make([]byte, bufSize+bufSize/2))
_, _ = w.Write(make([]byte, bufSize+bufSize/2))

op, r, err := rc.NextReader()
if op != BinaryMessage || err != nil {
Expand All @@ -438,7 +438,7 @@ func TestEOFBeforeFinalFrame(t *testing.T) {
func TestWriteAfterMessageWriterClose(t *testing.T) {
wc := newTestConn(nil, &bytes.Buffer{}, false)
w, _ := wc.NextWriter(BinaryMessage)
io.WriteString(w, "hello")
_, _ = io.WriteString(w, "hello")
if err := w.Close(); err != nil {
t.Fatalf("unxpected error closing message writer, %v", err)
}
Expand All @@ -448,7 +448,7 @@ func TestWriteAfterMessageWriterClose(t *testing.T) {
}

w, _ = wc.NextWriter(BinaryMessage)
io.WriteString(w, "hello")
_, _ = io.WriteString(w, "hello")

// close w by getting next writer
_, err := wc.NextWriter(BinaryMessage)
Expand All @@ -473,13 +473,13 @@ func TestReadLimit(t *testing.T) {

// Send message at the limit with interleaved pong.
w, _ := wc.NextWriter(BinaryMessage)
w.Write(message[:readLimit-1])
wc.WriteControl(PongMessage, []byte("this is a pong"), time.Now().Add(10*time.Second))
w.Write(message[:1])
_, _ = w.Write(message[:readLimit-1])
_ = wc.WriteControl(PongMessage, []byte("this is a pong"), time.Now().Add(10*time.Second))
_, _ = w.Write(message[:1])
w.Close()

// Send message larger than the limit.
wc.WriteMessage(BinaryMessage, message[:readLimit+1])
_ = wc.WriteMessage(BinaryMessage, message[:readLimit+1])

op, _, err := rc.NextReader()
if op != BinaryMessage || err != nil {
Expand Down Expand Up @@ -592,7 +592,7 @@ func TestBufioReadBytes(t *testing.T) {
rc := newConn(fakeNetConn{Reader: &b1, Writer: &b2}, true, len(m)-64, len(m)-64, nil, nil, nil)

w, _ := wc.NextWriter(BinaryMessage)
w.Write(m)
_, _ = w.Write(m)
w.Close()

op, r, err := rc.NextReader()
Expand Down Expand Up @@ -666,7 +666,7 @@ func TestConcurrentWritePanic(t *testing.T) {
w := blockingWriter{make(chan struct{}), make(chan struct{})}
c := newTestConn(nil, w, false)
go func() {
c.WriteMessage(TextMessage, []byte{})
_ = c.WriteMessage(TextMessage, []byte{})
}()

// wait for goroutine to block in write.
Expand All @@ -679,7 +679,7 @@ func TestConcurrentWritePanic(t *testing.T) {
}
}()

c.WriteMessage(TextMessage, []byte{})
_ = c.WriteMessage(TextMessage, []byte{})
t.Fatal("should not get here")
}

Expand All @@ -699,7 +699,7 @@ func TestFailedConnectionReadPanic(t *testing.T) {
}()

for i := 0; i < 20000; i++ {
c.ReadMessage()
_, _, _ = c.ReadMessage()
}
t.Fatal("should not get here")
}
2 changes: 1 addition & 1 deletion join_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ func TestJoinMessages(t *testing.T) {
wc := newTestConn(nil, &connBuf, true)
rc := newTestConn(&connBuf, nil, false)
for _, m := range messages {
wc.WriteMessage(BinaryMessage, []byte(m))
_ = wc.WriteMessage(BinaryMessage, []byte(m))
}

var result bytes.Buffer
Expand Down
4 changes: 3 additions & 1 deletion prepared_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ func TestPreparedMessage(t *testing.T) {
if tt.enableWriteCompression {
c.newCompressionWriter = compressNoContextTakeover
}
c.SetCompressionLevel(tt.compressionLevel)
if err := c.SetCompressionLevel(tt.compressionLevel); err != nil {
t.Fatal(err)
}

// Seed random number generator for consistent frame mask.
testRand.Seed(1234)
Expand Down
Loading

0 comments on commit 3810b23

Please sign in to comment.