More tests, clean up test log noise, fix a GOAWAY bug.
The bug, found via new tests: if the ReadFrames goroutine read a frame
from the Framer resulting in an error of type ConnectionError, the
code specific to handling ConnectionError wasn't getting to handle it.
We always assumed that all errors coming from the Framer were related
to connection errors (io.EOF, etc). So now fall through past the
normal frame-processing logic and handle all errors the same way. That
means we now respond with a GOAWAY frame to clients who sent us bogus
frames rejected by the framer level too.
diff --git a/server.go b/server.go
index d2d2756..ff94b80 100644
--- a/server.go
+++ b/server.go
@@ -403,7 +403,7 @@
for {
f, err := sc.framer.ReadFrame()
if err != nil {
- sc.readFrameErrCh <- err // BEFORE the close
+ sc.readFrameErrCh <- err
close(sc.readFrameCh)
return
}
@@ -788,29 +788,33 @@
// processFrameFromReader returns whether the connection should be kept open.
func (sc *serverConn) processFrameFromReader(fg frameAndGate, fgValid bool) bool {
sc.serveG.check()
+ var clientGone bool
+ var err error
if !fgValid {
- err := <-sc.readFrameErrCh
+ err = <-sc.readFrameErrCh
if err == ErrFrameTooLarge {
sc.goAway(ErrCodeFrameSize)
return true // goAway will close the loop
}
- if err != io.EOF {
- errstr := err.Error()
- if !strings.Contains(errstr, "use of closed network connection") {
- sc.logf("client %s stopped sending frames: %v", sc.conn.RemoteAddr(), errstr)
- }
+ clientGone = err == io.EOF || strings.Contains(err.Error(), "use of closed network connection")
+ if clientGone {
+ // TODO: could we also get into this state if
+ // the peer does a half close
+ // (e.g. CloseWrite) because they're done
+ // sending frames but they're still wanting
+ // our open replies? Investigate.
+ return false
}
- // TODO: could we also get into this state if the peer does a half close (e.g. CloseWrite)
- // because they're done sending frames but they're still wanting our open replies?
- // Investigate.
- return false
}
- f := fg.f
- sc.vlogf("got %v: %#v", f.Header(), f)
- err := sc.processFrame(f)
- fg.g.Done() // unblock the readFrames goroutine
- if err == nil {
- return true
+
+ if fgValid {
+ f := fg.f
+ sc.vlogf("got %v: %#v", f.Header(), f)
+ err = sc.processFrame(f)
+ fg.g.Done() // unblock the readFrames goroutine
+ if err == nil {
+ return true
+ }
}
switch ev := err.(type) {
@@ -825,7 +829,11 @@
sc.goAway(ErrCode(ev))
return true // goAway will handle shutdown
default:
- sc.logf("disconnection due to other error: %v", err)
+ if !fgValid {
+ sc.logf("disconnecting; error reading frame from client %s: %v", sc.conn.RemoteAddr(), err)
+ } else {
+ sc.logf("disconnection due to other error: %v", err)
+ }
}
return false
}
