Cancel Handler context when connection closes

[samherrmann]

The changes in this pull request cancel the context that is given to the Handler when the connection is closed.

Before this change, the user of this library had to do something like this:

ctx, cancel := context.WithCancel(context.Background())
conn := jsonrpc2.NewConn(ctx, myStream, myHandler)

// Cancel context when the connection is disconnected.
go func() {
  <-conn.DisconnectNotify()
  cancel()
}()

Having to manually cancel the context when the connection is disconnected is unexpected. For comparison, the following is what the documentation states for the request context in the net/http package:

For incoming server requests, the context is canceled when the client's connection closes, the request is canceled (with HTTP/2), or when the ServeHTTP method returns.

[samherrmann]

I delivered the newClientServer function in #67. I regret naming it that (despite it only existing in the jsonrpc2_test package and not publicly facing) because JSON-RPC is bidirectional where both sides can be the client and the server. I therefore renamed this function to Pipe to mirror the name of the underlying net.Pipe.

While this change is off-topic, I included it in this pull request because the new test uses this function. If you'd like me to break it out then please let me know.

[keegancsmith]

LGTM. I think this is a good change, even though technically it changes the behaviour. One minor bit of feedback. Do you think it makes sense to remove the disconnect channel and replace it with the context channel?

[samherrmann]

Do you think it makes sense to remove the disconnect channel and replace it with the context channel?

Great question! The short version of my answer is that we should not replace the
disconnect channel with the context channel. And here is the long version:

I realized that if the user cancels the context that they're giving to NewConn,
that it has no impact on the connection. So the question is, if the context
doesn't affect the connection, then why is it an argument of NewConn? To
address that question I think we have the following options:

1. Remove the context argument from NewConn.
2. Leave the signature of NewConn as is but document that the context doesn't
   do anything. For this option we'd have to disconnect the context given to the
   Handler from the context given to NewConn.
3. Close Conn when the channel of the context given to NewConn is closed.

Personally I prefer option 3 because that is the behavior I would expect. I added a
second commit to this PR , named Close Conn when given context is done, that
implements this option. If we agree on option 3, then we can come back to your
question regarding the context channel replacing the disconnect channel. Let
me explain why I advice against doing that with the following code snippet:

ctx, cancel := context.WithCancel(context.Background())
conn := jsonrpc2.NewConn(ctx, myStream, myHandler)

cancel()
<-conn.DisconnectNotify()
conn.Close() // should return jsonrpc2.ErrClosed

If we replaced the disconnect channel with the context channel, that would
mean that conn.DisconnectNotify() would return the context channel.
Consequently, this code snippet would be subject to a race condition when the
user cancels their provided context. The race condition is that conn.Close()
may execute before or after the internal goroutine has a chance to close the
connection. Depending on the order, conn.Close() may return nil or
jsonrpc2.ErrClosed. By having conn.DisconnectNotify() return the
disconnect channel, we can control that the channel is only closed after the
connection is in fact closed, and that conn.Close() in the scenario above
always returns jsonrpc2.ErrClosed.

[keegancsmith]

Sorry for missing the update. I agree with the updated implementation. It makes sense to close if ctx is closed.

[keegancsmith]

right so this is safe since we will always call cancel when done. lgtm

[samherrmann]

Sorry for missing the update. I agree with the updated implementation. It makes sense to close if ctx is closed.

No worries, I'm sure you're busy. 🙂 Thank you very much for reviewing and merging, @keegancsmith!