Close on WriteResponse errors #15
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Introduce tests covering both WriteRequest and WriteResponse paths to ensure that when inlining the implementation of MsgpackCodec.write in the next commit, that it behaves correctly. The tests validate the following functionality: - proper round-trip encoding/decoding of RPC requests and responses when using buffered and unbuffered I/O - the codec decodes even when a nil target is provided - subsequent read/write operations immediately return io.EOF after the codec is closed
Remove the shared write() helper function by inlining its logic into both WriteResponse and WriteRequest.
Modify WriteResponse to call cc.Close() immediately if an error occurs while encoding the response header, encoding the body, or flushing the buffered writer. This prevents further use of a codec that encountered an error, ensuring that subsequent operations return io.EOF. This is *not* needed for WriteRequest as net/rpc propagates that error to the caller, and a comment has been added to that method explaining the difference.
schmichael
approved these changes
Feb 7, 2025
schmichael
added a commit
to hashicorp/go-msgpack
that referenced
this pull request
Feb 7, 2025
See hashicorp/net-rpc-msgpackrpc#15 for details.
schmichael
added a commit
to hashicorp/go-msgpack
that referenced
this pull request
Feb 7, 2025
See hashicorp/net-rpc-msgpackrpc#15 for details.
tgross
pushed a commit
to hashicorp/go-msgpack
that referenced
this pull request
Feb 20, 2025
Port the fix from hashicorp/net-rpc-msgpackrpc#15 **For consistency only.** To the best of my knowledge no HashiCorp tools use this code. They only use the serialization code from this package and all use hashicorp/net-rpc-msgpackrpc for RPC. Also add @hashicorp/nomad-eng to CODEOWNERS as in hashicorp/go-sockaddr#75 Team will also need repo write access.
schmichael
added a commit
to schmichael/net-rpc-msgpackrpc
that referenced
this pull request
Feb 20, 2025
Add @hashicorp/nomad-eng as owners. Nomad needs to merge hashicorp#15 to fix hashicorp/nomad#23305 @hashicorp/nomad-eng needs write access as well.
Merged
tgross
approved these changes
Feb 24, 2025
tgross
added a commit
that referenced
this pull request
Feb 24, 2025
Update go-msgpack to pick up the bug fix described in hashicorp/go-msgpack#26, which mirrors the work done here in #15
tgross
added a commit
that referenced
this pull request
Feb 24, 2025
Update go-msgpack to pick up the bug fix described in hashicorp/go-msgpack#26, which mirrors the work done here in #15 Also bump go version for CI to a supported version.
tgross
added a commit
that referenced
this pull request
Feb 24, 2025
Update go-msgpack to pick up the bug fix described in hashicorp/go-msgpack#26, which mirrors the work done here in #15 Also bump go version for CI to a supported version.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It turns out when
net/rpccallsWriteResponse, it doesn't do anything with the error! (it does correctly propagate errors whenWriteRequestis called though)This means this codec needs to handle connection disposal, because there's no way for someone using
net/rpcto find out about the error and handle it- we're in the twilight zone.Additionally, because
net/rpcis frozen upstream, there is a lack of desire to fix even the logging aspect, let alone the underlying defect: golang/go#71559The
WriteResponseandWriteRequestmethods both use the same underlyingwritemethod, so this fix took three commits-writeintoWriteResponseandWriteRequestThis puts an end to two years of "spooky behaviour at a distance" on our large and geographically distributed Nomad cluster.
I will also be opening a PR against hashicorp/consul-net-rpc for the same code change.
requested tags: @schmichael, @tgross
Fixes: hashicorp/nomad#23305