This is an automated email from the ASF dual-hosted git repository. kenhuuu pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit 3b847d771cf9674543e26554bed23a3e9bc066f5 Author: Ken Hu <[email protected]> AuthorDate: Tue Jun 23 08:44:32 2026 -0700 Terminate the chunked response on serializer failure CTR The terminal LastHttpContent clears the channel in-use flag; skipping it hangs the client and poisons the keep-alive connection. Termination previously rested on the body path not throwing. Make it structural: complete() in the eval finally, writeError terminates in finally and swallows Throwable, header sent before body, and STREAMING set only after writeHeader succeeds. Assisted-by: Claude Code:claude-opus-4-8 --- .../server/handler/HttpGremlinEndpointHandler.java | 10 ++++--- .../server/handler/HttpResponseCoordinator.java | 31 +++++++++++++++------- 2 files changed, 29 insertions(+), 12 deletions(-) diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpGremlinEndpointHandler.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpGremlinEndpointHandler.java index 2248044a28..7b78a31b77 100644 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpGremlinEndpointHandler.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpGremlinEndpointHandler.java @@ -272,12 +272,16 @@ public class HttpGremlinEndpointHandler extends SimpleChannelInboundHandler<Requ // failures that follow this will show up in the response body instead. coordinator.writeHeader(createResponseHeaders(ctx, serializer, requestCtx).toArray(CharSequence[]::new)); sendHttpContents(ctx, requestCtx, coordinator); - // Idempotent terminal call: if the data path already terminated the response, complete() is a no-op - // via its COMPLETED short-circuit. Otherwise it writes the terminal LastHttpContent. - coordinator.complete(HttpResponseStatus.OK, ""); } catch (Throwable t) { coordinator.writeError(formErrorResponseMessage(t, requestMessage)); } finally { + // Idempotent terminal backstop: if the data or error path already terminated the response, complete() + // is a no-op via its COMPLETED short-circuit. It runs in finally — not at the end of the try — so the + // chunked stream is still terminated if an unchecked throwable escaped the catch block itself (for + // example formErrorResponseMessage throwing while building the error). This guarantees the terminal + // LastHttpContent is always written, so the client never hangs and the keep-alive channel clears. + coordinator.complete(HttpResponseStatus.OK, ""); + timerContext.stop(); // There is a race condition that this query may have finished before the timeoutFuture was created, diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpResponseCoordinator.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpResponseCoordinator.java index c98dca641c..2fc6604535 100644 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpResponseCoordinator.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpResponseCoordinator.java @@ -31,7 +31,6 @@ import org.apache.tinkerpop.gremlin.server.util.GremlinError; import org.apache.tinkerpop.gremlin.util.MessageSerializer; import org.apache.tinkerpop.gremlin.util.message.ResponseMessage; import org.apache.tinkerpop.gremlin.util.ser.SerTokens; -import org.apache.tinkerpop.gremlin.util.ser.SerializationException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -133,8 +132,12 @@ final class HttpResponseCoordinator { if (firstChunk && terminal) { chunk = serializer.serializeResponseAsBinary(responseMessage, nettyContext.alloc()); } else if (firstChunk) { - state = State.STREAMING; + // Serialize the header chunk BEFORE transitioning to STREAMING. If writeHeader throws (the very + // first streamed element fails to serialize), state stays NOT_STARTED so the catch below routes to + // writeError while still pre-stream — which serializes a full, parseable error message rather than + // a footer-only body with no preceding header bytes. chunk = serializer.writeHeader(responseMessage, nettyContext.alloc()); + state = State.STREAMING; } else { chunk = serializer.writeFooter(responseMessage, nettyContext.alloc()); } @@ -165,19 +168,29 @@ final class HttpResponseCoordinator { final ChannelHandlerContext ctx = context.getChannelHandlerContext(); try { + // Send the header before serializing the body. An error response that has not yet emitted a header carries + // the error status code on the header line, matching the prior behavior of HttpHandlerUtil.writeError. Doing + // this first means that if serialization then throws, the finally below still terminates with a well-formed + // (body-less) error rather than a bare LastHttpContent with no header line. Mid-stream this is a no-op since + // the header was already sent during the data phase. + ensureHeaderSent(responseMessage.getStatus().getCode(), HttpHeaderNames.CONTENT_TYPE, contentType); + final ByteBuf byteBuf = state == State.STREAMING ? serializer.writeErrorFooter(responseMessage, ctx.alloc()) : serializer.serializeResponseAsBinary(responseMessage, ctx.alloc()); - - // an error response that has not yet emitted a header carries the error status code on the header line, - // matching the prior behavior of HttpHandlerUtil.writeError. - ensureHeaderSent(responseMessage.getStatus().getCode(), HttpHeaderNames.CONTENT_TYPE, contentType); - ctx.writeAndFlush(new DefaultHttpContent(byteBuf)); + } catch (Throwable t) { + // Catch Throwable (not just SerializationException) and swallow it: a custom TypeSerializer throwing an + // unchecked RuntimeException/Error must not propagate out of writeError (it would be uncaught on the + // timeout/scheduler thread) nor skip the terminal write in the finally. The error body could not be + // serialized, but the stream is still terminated so the client does not hang and the keep-alive channel's + // in-use flag clears. + logger.warn("Unable to serialize ResponseMessage: {} ", responseMessage, t); + } finally { + // Idempotent terminal write: runs whether or not the body serialized, so the chunked stream is always + // ended. Mirrors complete()'s finally-based backstop in the eval task. writeTerminal(responseMessage.getStatus().getCode(), responseMessage.getStatus().getException()); state = State.COMPLETED; - } catch (SerializationException se) { - logger.warn("Unable to serialize ResponseMessage: {} ", responseMessage); } }
