Behavioral test fixes and error-handling consistency across GLVs#3519
Open
Cole-Greer wants to merge 10 commits into
Open
Behavioral test fixes and error-handling consistency across GLVs#3519Cole-Greer wants to merge 10 commits into
Cole-Greer wants to merge 10 commits into
Conversation
- Add a RequestTimeout option wired to http.Transport.ResponseHeaderTimeout so a server that accepts the connection but never responds surfaces a timeout instead of hanging (tinkerpop-y32). - Treat an empty HTTP response body as an error instead of returning an empty result set (tinkerpop-zar). - Unskip and tighten the corresponding behavioral tests.
- Add a requestTimeout option bounding time-to-first-byte (cleared once response headers arrive so slow streams are unaffected); reject with a ResponseError when the server never responds (tinkerpop-uvl). - Wrap low-level fetch/transport errors in a Gremlin-aware ResponseError with an actionable message, preserving the original error as the cause (tinkerpop-1fn). - Tighten behavioral test assertions.
- Release the aiohttp response on error so a half-closed connection is evicted and the same client recovers after an empty response body (tinkerpop-sti). - Wrap low-level aiohttp transport errors in GremlinConnectionError with an actionable message on both the request and response paths (tinkerpop-1fn). - Use a distinct "Server returned an empty response body" message for the empty-body case (tinkerpop-7s3). - Tighten behavioral test assertions.
- Add ResponseDeserializationException and wrap response-deserialization failures so a malformed response yields a single consistent exception type instead of a non-deterministic IOException/KeyNotFoundException (tinkerpop-9t5). - Leave transport-level HttpIOException (premature connection close) unwrapped so the partial-content-close path keeps its transport-error semantics. - Update behavioral test assertions.
…driver) - Produce a clear "Server returned an empty response body" error instead of a bare EOFException when the response body is empty (tinkerpop-28f). - Add hermetic HttpGremlinRequestEncoder tests validating the User-Agent header (present/absent) and that per-request settings (timeout, batchSize, materializeProperties) survive encoding into the outgoing request (tinkerpop-3tw.8).
Replace the newly-added ResponseDeserializationException with a second ResponseException constructor, keeping a single exception type for driver-level response failures (server-reported status errors and local deserialization failures) and matching the reuse-over-new-type approach taken in gremlin-javascript for the analogous case.
The requestTimeout option added to gremlin-go and gremlin-javascript in this branch (a time-to-first-byte / whole-request deadline) duplicates and conflicts with the readTimeout / readTimeoutMillis option that master's cross-GLV connection-options standardization already established as the canonical timeout knob. readTimeout is streaming-safe (armed per read/chunk) and already covers the server-never-responds scenario, since the very first read of the response never succeeds. - gremlin-go: remove connectionSettings.requestTimeout and ClientSettings.RequestTimeout; drop the http.Transport.ResponseHeaderTimeout wiring. TestShouldTimeoutWhenServerNeverResponds now sets ReadTimeout. - gremlin-javascript: remove ConnectionOptions.requestTimeout and the AbortController-based timeout wrapping in #makeHttpRequest, keeping the transport-error-wrapping behavior. The behavioral test now configures readTimeoutMillis and asserts on the generic connection-closed message.
This test was written against APIs that no longer exist on the current gremlin-driver main sources (org.apache.tinkerpop.gremlin.driver.interceptor. PayloadSerializingInterceptor, HttpGremlinRequestEncoder's pre-bulkResults constructor, RequestOptions.Builder.timeout()/getTimeout(), and Tokens.TIMEOUT_MS), all superseded by master's connection-options standardization work. It was blocking test-compile on gremlin-driver.
Drop the ResponseError wrapping around fetch/response failures in Connection#makeHttpRequest and #handleResponse, reverting to raw transport errors bubbling up unwrapped. Skip should timeout when server never responds: readTimeoutMillis maps only to undici's bodyTimeout, which doesn't start ticking until response parsing begins, so it never fires for a server that sends nothing at all.
- ClientBehaviorIntegrateTest.shouldHandleEmptyResponseBody: the empty-response wrapping added an extra RuntimeException layer, shifting the cause chain by one. Use ExceptionHelper.getRootCause() instead of asserting a fixed depth, and check for the new message text. - gremlin-python: narrow _TRANSPORT_ERRORS from ClientConnectionError to ClientOSError. ClientConnectionError structurally also covers ServerTimeoutError/SocketTimeoutError, so a connect/write-phase timeout was being wrapped as GremlinConnectionError instead of surfacing as a timeout. ClientOSError excludes the timeout branch by definition rather than via an added special case.
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
1 participant
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.
This PR is a followup to the audit findings from #3436.
Improves error-handling consistency for adversarial HTTP scenarios (empty responses, malformed responses, dead connections) across the Gremlin drivers, and reconciles a few timeout-related test gaps against master's connection-options standardization work.
Java (gremlin-driver)
Go (gremlin-go)
Problem: Same empty-body issue as Java — the driver silently returned an empty result set instead of an error.
Fix: Empty response bodies now produce a clear error.
Problem: TestShouldTimeoutWhenServerNeverResponds was skipped outright since the driver had no client-side read/request timeout at the time.
Fix: Un-skipped, now uses the standardized ReadTimeout option (confirmed it correctly bounds this case, since Go's per-read deadline is armed before the first read).
Python (gremlin_python)
Problem: Transport-level failures (server closes connection, partial response, empty body) leaked raw aiohttp exception types (ClientConnectionError, ClientPayloadError, ServerDisconnectedError) with no Gremlin-level context.
Fix: Wraps these in a new GremlinConnectionError with an actionable message.
Problem: A half-closed connection wasn't evicted from the pool after an empty response, so the client could keep reusing a dead connection.
Fix: Releases the response on error so the connection is evicted and the pool recovers.
.NET (Gremlin.Net)
JavaScript (gremlin-javascript)
VOTE +1