search

sourcegraph / jsonrpc2

Package jsonrpc2 provides a client and server implementation of JSON-RPC 2.0 (http://www.jsonrpc.org/specification)
MIT License
195 stars 62 forks source link

conn: do not lock sending when closing #70

Closed keegancsmith closed 1 year ago

keegancsmith commented 1 year ago

The sending mutex may be blocked due to the underlying conn blocking. If that happens then we can't call close since close also attempts to hold the sending mutex. Sending mutex is only used for serializing sends and doesn't protect the fields close reads/writes. I believe we introduced locking it so we would return ErrClosed. This commit instead introduces a check in send which rechecks if we have since closed while attempting to send.

Test Plan: expanded the test coverage

Part of https://github.com/sourcegraph/jsonrpc2/issues/69

keegancsmith commented 1 year ago

@samherrmann can't seem to add you as a reviewer, but mind taking a look?

samherrmann commented 1 year ago

LGTM.

Are any of the new test cases expected to fail with the previous code? If I take your changes in conn_test.go but not the changes in conn.go then go test -race ./... still passes.

keegancsmith commented 1 year ago

yeah, I didn't introduce a test case which caused the old failure. Honestly I won't have time to tinker with that any time soon, but I imagine you can reproduce by creating io.ReadWriteCloser which blocks on Read and Write until Close is called.

The test cases I introduced were more to ensure I was correctly understanding the reason the send lock in close was introduced + ensuring we tested all possible call code paths.