Make HttpRequestHandlerImpl.handle() async when possible (fixes #259) #268
Conversation
| @@ -27,25 +28,36 @@ | |||
| } | |||
|
|
|||
| public GraphQLQueryResult query(GraphQLInvocationInput invocationInput) { | |||
| return queryAsync(invocationInput).join(); | |||
There was a problem hiding this comment.
I preserved the original signatures for binary compatibility.
This one is also still used by AbstractGraphQLHttpServlet.executeQuery() for JMX.
| .map(CompletableFuture::join) | ||
| .collect(toList()); | ||
| @SuppressWarnings({"unchecked", "rawtypes"}) | ||
| private <T> CompletableFuture<List<T>> sequence(List<CompletableFuture<T>> futures) { |
There was a problem hiding this comment.
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().
graphql-java-kickstart/src/main/java/graphql/kickstart/execution/GraphQLInvoker.java
Show resolved
Hide resolved
graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestHandlerImpl.java
Outdated
Show resolved
Hide resolved
graphql-java-kickstart/src/main/java/graphql/kickstart/execution/GraphQLInvoker.java
Show resolved
Hide resolved
| @@ -48,37 +51,61 @@ 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()) { | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I added JsonProcessingException instead of IOException.
graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestHandlerImpl.java
Outdated
Show resolved
Hide resolved
graphql-java-kickstart/src/main/java/graphql/kickstart/execution/GraphQLInvoker.java
Outdated
Show resolved
Hide resolved
|
@olim7t Is this PR really still a draft or ready to merge from your POV? |
|
@olim7t Any update on this PR? |
|
Sorry @oliemansm I completely missed your comments. This is not ready yet. The part about modifying the servlets that I mentioned in my initial comment is not done:
It feels wasteful to keep that executor if To be completely honest, I won't have time to finish this PR: I ended up dropping the dependency to graphql-java-servlet and switching to a custom implementation in my project (not solely because of this issue; my use case is a bit peculiar). But if someone else is willing to address the servlet part, the basics are here. |
# Conflicts: # graphql-java-kickstart/src/main/java/graphql/kickstart/execution/GraphQLInvoker.java # graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestHandlerImpl.java
Not complete but I wanted to open the discussion before I modify the servlets: I would personally have
AbstractGraphQLHttpServlet.doRequestAsync()invokehandler.handle()directly, and deprecateconfiguration.getAsyncExecutor()andconfiguration.isAsyncServletModeEnabled(). This means the query will execute synchronously if the data fetchers are not async -- or if theHttpServletRequestdoesn't support async for some reason. But I agree with @artem-ag that it's probably not that useful to reschedule on another thread pool in that case.I've tried to follow the existing code style, let me know if anything is off.