Improvements for connection desynchronization and hardening of error handling with connections#1998
Draft
rozza wants to merge 4 commits into
Draft
Improvements for connection desynchronization and hardening of error handling with connections#1998rozza wants to merge 4 commits into
rozza wants to merge 4 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR (JAVA-6120) hardens InternalStreamConnection against connection desynchronization and improves robustness when underlying stream operations fail, ensuring problematic connections are closed and command monitoring emits the correct failure events.
Changes:
- Close connections on desynchronization conditions (e.g.,
MongoInternalExceptionsuch as responseTo mismatch) while keeping connections reusable for fully-read command error responses. - Ensure async header/body parsing failures close the connection and release buffers before closing the underlying stream.
- Add unit tests covering async/sync failure modes (write/read errors, header/body parsing failures, responseTo mismatches, command monitoring event correctness).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| driver-core/src/main/com/mongodb/internal/connection/InternalStreamConnection.java | Closes desynced connections, broadens exception catching to include Throwable for stream I/O, and guarantees buffer release before closing in async receive paths. |
| driver-core/src/test/unit/com/mongodb/internal/connection/InternalStreamConnectionTest.java | Adds targeted unit tests for desync/error-handling behavior and command monitoring event emission. |
| .agents/references/testing-guide.md | Documents additional testing conventions relevant to these new tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rozza
force-pushed
the
JAVA-6210
branch
2 times, most recently
from
f80da3f to
83eb39a
Compare
A responseTo mismatch indicates the stream is desynchronized, so the connection must be closed to prevent it returning to the pool. Command error responses (ok: 0) leave the stream intact and do not close the connection. Command monitoring now receives exactly one started and one terminal event in all mismatch scenarios, on both sync and async paths: the commandSuccessful flag is set only after the succeeded event was actually emitted, and the async path sends the failed event for any failure when no succeeded event went out, matching the sync path. Response buffers are released before the connection is closed: NettyStream.close() requires all buffers it handed out to have been released already. JAVA-6210
Ensure all error paths close the connection to prevent pooled connections from retaining unread response data: - sendMessage (sync): catch Throwable instead of Exception - sendMessageAsync: catch Throwable instead of Exception - readAsync: catch Throwable instead of Exception - MessageHeaderCallback: close on header parsing failure - MessageCallback: close on body read failure and body parsing failure - MessageCallback: deliver the parsed response outside the try block so a throwing downstream callback cannot close a healthy connection Header and body buffers are released before the connection is closed: NettyStream.close() requires all buffers it handed out to have been released already. Errors are translated to MongoInternalException, consistent with the pre-existing receiveResponseBuffers behavior. JAVA-6210
Capture conventions surfaced during review: import types rather than inline fully-qualified names, prefer atomics over array wrappers for state captured by lambdas, assert the most specific exception type, and pair side-effect tests with their negative counterparts. JAVA-6210
When reading an OP_COMPRESSED response, the uncompressed buffer allocated via getBuffer(...) leaked if uncompress, flip, or ReplyHeader parsing threw before ResponseBuffers took ownership, on both the sync (receiveResponseBuffers) and async (MessageCallback) paths. Release the buffer explicitly on those failure paths. JAVA-6210
This file contains hidden or 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
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.
JAVA-6120