[GitHub] [tinkerpop] dimas-b commented on a change in pull request #1168: TINKERPOP-2246 Use ResponseHandlerContext to ensure single final response to client

[GitBox <gi...@apache.org>]

dimas-b commented on a change in pull request #1168: TINKERPOP-2246 Use ResponseHandlerContext to ensure single final response to client
URL: https://github.com/apache/tinkerpop/pull/1168#discussion_r307291434
 
 

 ##########
 File path: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/AbstractEvalOpProcessor.java
 ##########
 @@ -191,45 +193,21 @@ protected AbstractEvalOpProcessor(final boolean manageTransactions) {
      * A generalized implementation of the "eval" operation.  It handles script evaluation and iteration of results
      * so as to write {@link ResponseMessage} objects down the Netty pipeline.  It also handles script timeouts,
      * iteration timeouts, metrics and building bindings.  Note that result iteration is delegated to the
-     * {@link #handleIterator} method, so those extending this class could override that method for better control
-     * over result iteration.
-     *
-     * @param context The current Gremlin Server {@link Context}
+     * {@link #handleIterator(Context, Iterator)} method, so those extending this class could override that method for
+     * better control over result iteration.
+     *  @param ctx The current Gremlin Server {@link Context}. This handler ensures that only a single final
+     *            response is sent to the client.
      * @param gremlinExecutorSupplier A function that returns the {@link GremlinExecutor} to use in executing the
      *                                script evaluation.
      * @param bindingsSupplier A function that returns the {@link Bindings} to provide to the
-     *                         {@link GremlinExecutor#eval} method.
-     * @see #evalOpInternal(ResponseHandlerContext, Supplier, BindingSupplier)
+ *                         {@link GremlinExecutor#eval} method.
      */
-    protected void evalOpInternal(final Context context, final Supplier<GremlinExecutor> gremlinExecutorSupplier,
+    protected void evalOpInternal(final Context ctx, final Supplier<GremlinExecutor> gremlinExecutorSupplier,
                                   final BindingSupplier bindingsSupplier) throws OpProcessorException {
-        final ResponseHandlerContext rhc = new ResponseHandlerContext(context);
-        try {
-            evalOpInternal(rhc, gremlinExecutorSupplier, bindingsSupplier);
-        } catch (Exception ex) {
-            // Exceptions may occur on after the script started executing, therefore corresponding errors must be
-            // reported via the ResponseHandlerContext.
-            logger.warn("Unable to process script evaluation request: " + ex, ex);
-            rhc.writeAndFlush(ResponseMessage.build(context.getRequestMessage())
-                    .code(ResponseStatusCode.SERVER_ERROR)
-                    .statusAttributeException(ex)
-                    .statusMessage(ex.getMessage()).create());
-        }
-    }
-
-    /**
-     * A variant of {@link #evalOpInternal(Context, Supplier, BindingSupplier)} that is suitable for use in situations
-     * when multiple threads may produce {@link ResponseStatusCode#isFinalResponse() final} response messages
-     * concurrently.
-     * @see #evalOpInternal(Context, Supplier, BindingSupplier)
-     */
-    protected void evalOpInternal(final ResponseHandlerContext rhc, final Supplier<GremlinExecutor> gremlinExecutorSupplier,
 
 Review comment:
   We are keeping `ResponseHandlerContext` as a deprecated class, but we are removing this `evalOpInternal` overload? If we intend to keep `ResponseHandlerContext` for backward compatibility would we also need to keep the methods that use it as parameters?
   
   Sorry, I did not review very closely, perhaps I missed something in this regard.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services