Fix underlying connection not being closed on protocol error #64
Conversation
Before this commit, the underlying connection of `Conn` was not being closed when a protocol error was encountered. This behavior contradicted with `Conn.DisconnectNotify()` because it reported that the underlying connection was being closed. Additionally, the underlying connection was now orphaned because `Conn` was no longer processing any of the subsequent requests. With this commit, the underlying connection is now being closed when a protocol error is encountered, matching what `Conn.DisconnectNotify()` has already been reporting.
The purpose of this commit is to consolidate assertions that are required for multiple tests related to `Conn.DisconnectNotify` into a single function.
|
Let me know if you wish to keep the change set of this PR to a minimum and remove the second commit. |
| @@ -366,11 +366,10 @@ type Conn struct { | |||
|
|
|||
| h Handler | |||
|
|
|||
| mu sync.Mutex | |||
| shutdown bool | |||
| closing bool | |||
There was a problem hiding this comment.
Since there is now a single closing procedure, there doesn't appear to be the need for a closing AND shutdown state anymore.
| func (readWriteCloser) Close() error { return nil } | ||
| t.Run("EOF", func(t *testing.T) { | ||
| connA, connB := net.Pipe() | ||
| c := jsonrpc2.NewConn(context.Background(), jsonrpc2.NewPlainObjectStream(connB), nil) |
There was a problem hiding this comment.
I switched from jsonrpc2.NewBufferedStream(conn, jsonrpc2.VarintObjectCodec{}), nil) to jsonrpc2.NewPlainObjectStream(conn) solely because the latter looks cleaner/shorter. The type of stream being used shouldn't impact the results of the these tests. I am happy to switching back to the former though if you prefer.
| @@ -314,105 +314,88 @@ type noopHandler struct{} | |||
|
|
|||
| func (noopHandler) Handle(ctx context.Context, conn *jsonrpc2.Conn, req *jsonrpc2.Request) {} | |||
|
|
|||
| type readWriteCloser struct { | |||
| read, write func(p []byte) (n int, err error) | |||
| } | |||
There was a problem hiding this comment.
Instead of modifying readWriteCloser so that it would allow us to test if it's been closed, I just used net.Pipe() from std lib instead.
| if err != io.ErrUnexpectedEOF && !closing { | ||
| c.logger.Printf("jsonrpc2: protocol error: %v\n", err) | ||
| } | ||
| close(c.disconnect) |
There was a problem hiding this comment.
This line is the source of the bug that's being fixed. Before, close(c.disconnect) was being called here but c.stream.Close() was not. That means that code that uses this library was being lied to if they were listening to Conn.DisconnectNotify(). c.stream.Close() was only being called if Conn was explicitly being closed by Conn.Close().
I rearranged the code so that the "closing" procedure is the same regardless if the cause is an error in Conn.readMessages or if the cause is a call to Conn.Close(). This should ensure that the state of the underlying connection matches what Conn.DiconnectNotify() reports.
From a high level, this is what it looks like:
func (c *Conn) readMessages() {
var err error
for err == nil {
// read from underlying connection
}
// for-loop exits when an error is encountered.
c.close(err)
}
func (c *Conn) Close() error {
return c.close(nil)
}
func (c *Conn) close(cause err) error {
// Common closing procedure that closes the underlying connection and also closes the
// `Conn.disconnect` channel.
}
jsonrpc2.go
Outdated
| } | ||
|
|
||
| if err := c.stream.Close(); err != nil { | ||
| return err |
There was a problem hiding this comment.
we probably still want to call close(c.disconnect) and c.closed = true even if closing the stream fails.
There was a problem hiding this comment.
Agreed. Addressed in 0c9de81.
| closing := c.closing | ||
| if err == io.EOF { | ||
| if closing { | ||
| err = ErrClosed |
There was a problem hiding this comment.
It doesn't seem like in the new codepath we ever set err to ErrClosed. Which means we don't inform c.pending ErrClosed. Given your recent diving into this code you likely have more context to know if this seems correct than I do right now. So is the new code path alright with respect to that?
There was a problem hiding this comment.
Excellent observation! I definitely overlooked this before you drew my attention to it. As it turns out, from what I can see, we don't actually need to send ErrClosed through the channel because the channel listener infers ErrClosed when the channel is being closed. Based on that, I'm coming to the conclusion that having the following two lines...
call.done <- ErrClosed
close(call.done)...is redundant and we can reduce the code to just close(call.done) (see 85075f0). A follow-on question that may arise from my conclusion is "what about errors other than ErrClosed? Now those don't get sent to c.pending". It seems to me that if a connection is being closed, the cause of that closure is not the concern of a pending request. If a pending request is being cancelled due to the connection being closed, then all it cares about is ErrClosed.
Now, this investigation made me pay more attention to TestConn_Close: waiting for response, which is the test that I believe is responsible for verifying the behavior discussed above. That made me discover an issue with the test, which I discuss here.
The changes in this commit arrange TestConn_Close to execute as intended by closing the connection after sending the request.
| t.Errorf("got error %v, want %v", err, jsonrpc2.ErrClosed) | ||
| } | ||
| close(done) | ||
| }() | ||
| if err := c.Close(); err != nil && err != jsonrpc2.ErrClosed { |
There was a problem hiding this comment.
I discovered that this test did not run as intended. It is my understanding that the intention of this test is to ensure that pending calls receive ErrClosed when the connection is closed. What was happening here though is that Close() was being called before Call() even had a chance to execute. The test passed because the results are the same, but the condition set up by the test is different.
There was a problem hiding this comment.
nice! Now this all LGTM. Thanks for the deep diving :)
|
@keegancsmith, thank you very much for the quick turnaround time on the review 🙂 |
Please expand the commit messages for details.