Skip to content

Make HttpRequestHandlerImpl.handle() async when possible (fixes #259) #268

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Dec 19, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import graphql.kickstart.execution.input.GraphQLBatchedInvocationInput;
import graphql.kickstart.execution.input.GraphQLInvocationInput;
import graphql.kickstart.execution.input.GraphQLSingleInvocationInput;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.CompletableFuture;
import lombok.AllArgsConstructor;
Expand All @@ -27,25 +28,36 @@ public CompletableFuture<ExecutionResult> executeAsync(GraphQLSingleInvocationIn
}

public GraphQLQueryResult query(GraphQLInvocationInput invocationInput) {
return queryAsync(invocationInput).join();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I preserved the original signatures for binary compatibility.

This one is also still used by AbstractGraphQLHttpServlet.executeQuery() for JMX.

}

public CompletableFuture<GraphQLQueryResult> queryAsync(GraphQLInvocationInput invocationInput) {
if (invocationInput instanceof GraphQLSingleInvocationInput) {
return GraphQLQueryResult.create(query((GraphQLSingleInvocationInput) invocationInput));
return executeAsync((GraphQLSingleInvocationInput)invocationInput).thenApply(GraphQLQueryResult::create);
}
GraphQLBatchedInvocationInput batchedInvocationInput = (GraphQLBatchedInvocationInput) invocationInput;
return GraphQLQueryResult.create(query(batchedInvocationInput));
return executeAsync(batchedInvocationInput).thenApply(GraphQLQueryResult::create);
}

private ExecutionResult query(GraphQLSingleInvocationInput singleInvocationInput) {
return executeAsync(singleInvocationInput).join();
private CompletableFuture<List<ExecutionResult>> executeAsync(GraphQLBatchedInvocationInput batchedInvocationInput) {
GraphQL graphQL = batchedDataLoaderGraphQLBuilder.newGraphQL(batchedInvocationInput, graphQLBuilder);
return sequence(
batchedInvocationInput.getExecutionInputs().stream()
.map(executionInput -> proxy.executeAsync(graphQL, executionInput))
.collect(toList()));
}

private List<ExecutionResult> query(GraphQLBatchedInvocationInput batchedInvocationInput) {
GraphQL graphQL = batchedDataLoaderGraphQLBuilder.newGraphQL(batchedInvocationInput, graphQLBuilder);
return batchedInvocationInput.getExecutionInputs().stream()
.map(executionInput -> proxy.executeAsync(graphQL, executionInput))
.collect(toList())
.stream()
.map(CompletableFuture::join)
.collect(toList());
@SuppressWarnings({"unchecked", "rawtypes"})
private <T> CompletableFuture<List<T>> sequence(List<CompletableFuture<T>> futures) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the JDK doesn't provide this common operation out of the box 😞
It's also a bit hacky to work around the vararg argument of allOf().

CompletableFuture[] futuresArray = futures.toArray(new CompletableFuture[0]);
return CompletableFuture.allOf(futuresArray).thenApply(aVoid -> {
List<T> result = new ArrayList<>();
for (CompletableFuture future : futuresArray) {
assert future.isDone(); // per the API contract of allOf()
result.add((T) future.join());
}
return result;
});
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
import graphql.kickstart.execution.input.GraphQLInvocationInput;
import graphql.kickstart.execution.input.GraphQLSingleInvocationInput;
import java.io.IOException;
import java.io.UncheckedIOException;
import java.util.concurrent.CompletableFuture;
import javax.servlet.AsyncContext;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import lombok.extern.slf4j.Slf4j;
Expand Down Expand Up @@ -48,37 +51,60 @@ public void handle(HttpServletRequest request, HttpServletResponse response) thr

private void execute(GraphQLInvocationInput invocationInput, HttpServletRequest request,
HttpServletResponse response) {
try {
GraphQLQueryResult queryResult = invoke(invocationInput, request, response);
if (request.isAsyncSupported()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated comment. as you are touching this already.
Do you mind changing line 41 to GraphQLException | IOException e? invocationInputParser.getGraphQLInvocationInput throws IOException (JsonProcessingException or JsonMappingException specifically) in case of unparseable input request. That results in #258

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are those the only IOException instances that can happen here? Just making sure that we're not also catching others that would qualify as legitimate server errors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Official method signature throws IOException. So it's hard to guarantee that. The use case right now is that servlet returns http 500 for bad requests that can't be deserialized. And that makes external attacks last longer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added JsonProcessingException instead of IOException.

AsyncContext asyncContext = request.startAsync(request, response);
asyncContext.setTimeout(configuration.getAsyncTimeout());
invoke(invocationInput, request, response)
.thenAccept(result -> writeResultResponse(result, request, response))
.exceptionally(t -> writeErrorResponse(t, response))
.thenAccept(aVoid -> asyncContext.complete());
} else {
try {
GraphQLQueryResult result = invoke(invocationInput, request, response).join();
writeResultResponse(result, request, response);
} catch (Throwable t) {
writeErrorResponse(t, response);
}
}
}

QueryResponseWriter queryResponseWriter = QueryResponseWriter.createWriter(queryResult, configuration.getObjectMapper(),
configuration.getSubscriptionTimeout());
private void writeResultResponse(GraphQLQueryResult queryResult, HttpServletRequest request,
HttpServletResponse response) {
QueryResponseWriter queryResponseWriter = QueryResponseWriter.createWriter(queryResult, configuration.getObjectMapper(),
configuration.getSubscriptionTimeout());
try {
queryResponseWriter.write(request, response);
} catch (Throwable t) {
response.setStatus(STATUS_BAD_REQUEST);
log.info("Bad GET request: path was not \"/schema.json\" or no query variable named \"query\" given");
log.debug("Possibly due to exception: ", t);
} catch (IOException e) {
// will be processed by the exceptionally() call below
throw new UncheckedIOException(e);
}
}

private GraphQLQueryResult invoke(GraphQLInvocationInput invocationInput, HttpServletRequest request,
private Void writeErrorResponse(Throwable t, HttpServletResponse response) {
response.setStatus(STATUS_BAD_REQUEST);
log.info("Bad GET request: path was not \"/schema.json\" or no query variable named \"query\" given");
log.debug("Possibly due to exception: ", t);
return null;
}

private CompletableFuture<GraphQLQueryResult> invoke(GraphQLInvocationInput invocationInput, HttpServletRequest request,
HttpServletResponse response) {
if (invocationInput instanceof GraphQLSingleInvocationInput) {
return graphQLInvoker.query(invocationInput);
return graphQLInvoker.queryAsync(invocationInput);
}
return invokeBatched((GraphQLBatchedInvocationInput) invocationInput, request, response);
}

private GraphQLQueryResult invokeBatched(GraphQLBatchedInvocationInput batchedInvocationInput,
private CompletableFuture<GraphQLQueryResult> invokeBatched(GraphQLBatchedInvocationInput batchedInvocationInput,
HttpServletRequest request,
HttpServletResponse response) {
BatchInputPreProcessor preprocessor = configuration.getBatchInputPreProcessor();
BatchInputPreProcessResult result = preprocessor.preProcessBatch(batchedInvocationInput, request, response);
if (result.isExecutable()) {
return graphQLInvoker.query(result.getBatchedInvocationInput());
return graphQLInvoker.queryAsync(result.getBatchedInvocationInput());
}

return GraphQLQueryResult.createError(result.getStatusCode(), result.getStatusMessage());
return CompletableFuture.completedFuture(GraphQLQueryResult.createError(result.getStatusCode(), result.getStatusMessage()));
}

}