Bound streaming reader timeout by readTimeout instead of a fixed 30s#3505
Open
kenhuuu wants to merge 1 commit into
Open
Bound streaming reader timeout by readTimeout instead of a fixed 30s#3505kenhuuu wants to merge 1 commit into
kenhuuu wants to merge 1 commit into
Conversation
The gremlin-driver streaming reader thread used a hardcoded 30s queue
poll that ignored the configured readTimeout — so a client with
readTimeout=0 ("no timeout", the default) was still cut off at 30s, and
the failure surfaced as a raw IOException rather than a typed timeout.
The reader's wait now derives from readTimeout: 0 blocks indefinitely
(matching the aggregated path), and a positive value arms a backstop set
longer than readTimeout so the pipeline ReadTimeoutHandler fires first
and terminates the response with a proper exception. ResultSet.markError
is now first-writer-wins so a read-timeout and end-of-stream racing on
the same request can't leave the recorded cause and the completed future
disagreeing.
This surfaced as flakiness in shouldProduceProperExceptionOnTimeout on
slow CI runners, where the reader repeatedly stalled on the 30s poll and
pushed the test past the build limit. A 5-minute per-test timeout is
added so a reader stall fails the test cleanly instead of hanging the
build.
Assisted-by: Claude Code:claude-opus-4-8
Contributor
|
I think there are CI failures related to the changes. VOTE +1 |
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.
The gremlin-driver streaming reader thread used a hardcoded 30s queue poll that ignored the configured readTimeout — so a client with readTimeout=0 ("no timeout", the default) was still cut off at 30s, and the failure surfaced as a raw IOException rather than a typed timeout.
The reader's wait now derives from readTimeout: 0 blocks indefinitely (matching the aggregated path), and a positive value arms a backstop set longer than readTimeout so the pipeline ReadTimeoutHandler fires first and terminates the response with a proper exception. ResultSet.markError is now first-writer-wins so a read-timeout and end-of-stream racing on the same request can't leave the recorded cause and the completed future disagreeing.
This surfaced as flakiness in shouldProduceProperExceptionOnTimeout on slow CI runners, where the reader repeatedly stalled on the 30s poll and pushed the test past the build limit. A 5-minute per-test timeout is added so a reader stall fails the test cleanly instead of hanging the build.
Review guide:
This most important part is that this PR determines how the streaming GraphBinary inputstream reader and readTimeout option need to combine together. Determine if the chosen path (have the InputStream respect the readTimeout) is the right path forward.
VOTE +1