GraphQLServletListener.onRequest() can't be guaranteed run before DataFetcher.get()

[SylarChen]

How to reproduce:
set threadlocal in GraphQLServletListener.onRequest()
get threadlocal in DataFetcher.get()
sometimes can't get the threadlocal in DataFetcher.get()

ENV：
graphql-java-servlet version: 11.1.1
graphql-java: 16.1
openJDK: 11
graphql java servlet async mode

[SylarChen]

code in HttpRequestInvokerImpl.invokeAndHandleAsync() may cause thread switch.
We need a way to wrap this runnable here, for transmitting data from GraphQLServletListener.onRequest() to DataFetcher.get() by threadlocal.

   asyncContext.start(
        () -> {
          FutureExecutionResult futureResult = invoke(invocationInput, request, response);
          futureHolder.set(futureResult);
          handle(futureResult, request, response, listenerHandler)
              .thenAccept(it -> asyncContext.complete());
        });

[SylarChen]

or any other suggestions ?

[oliemansm]

@SylarChen You could disable async mode (for now) then it should work as expected. But when async mode is enabled we indeed need a way to ensure onRequest is called in the correct phase.

[SylarChen]

share the solution here:

• wrap the asyncContext in Servlet Filter
• wrap the Runable object invoked by asyncContext
• BindWorkContextRunnable will save the variable get by threadloacal in constuctor.
• when run() is invoked, threadlocal will set this variable first, and release finally

public class GraphqlRequestWrapperFilter implements Filter {
    @Override
    public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException,
            ServletException {
        if (request instanceof HttpServletRequest) {
            chain.doFilter(new GraphqlServletWrapper((HttpServletRequest) request), response);
        } else {
            chain.doFilter(request, response);
        }
    }
    ......
}

class GraphqlServletWrapper extends HttpServletRequestWrapper {
    public GraphqlServletWrapper(HttpServletRequest request) {
        super(request);
    }
    @Override
    public AsyncContext startAsync() throws IllegalStateException {
        AsyncContext asyncContext = super.startAsync();
        return (asyncContext instanceof AsyncContextWrapper)
                ? asyncContext
                : new AsyncContextWrapper(asyncContext);
    }
    @Override
    public AsyncContext startAsync(ServletRequest request, ServletResponse response)
            throws IllegalStateException {
        AsyncContext asyncContext = super.startAsync(request, response);
        return (asyncContext instanceof AsyncContextWrapper)
                ? asyncContext
                : new AsyncContextWrapper(asyncContext);
    }
    ......
}

class AsyncContextWrapper implements AsyncContext {
    private AsyncContext asyncContext;
    public AsyncContextWrapper(AsyncContext asyncContext) {
        this.asyncContext = asyncContext;
    }
    @Override
    public void start(Runnable run) {
        this.asyncContext.start(new BindWorkContextRunnable(run));
    }
    ......
}

[SylarChen]

I see the latest version 12.0.0 add feature "customer executor configuration", this helps a lot.

but it seems async mode has other problem, see #403

please have a look.. @oliemansm

[oliemansm]

@SylarChen Is this issue still relevant with the "customer executor configuration" function and its fix for #403 ? Or can this be closed now?

[SylarChen]

@SylarChen Is this issue still relevant with the "customer executor configuration" function and its fix for #403 ? Or can this be closed now?

this can be closed now