Open gflores-jahia opened 3 months ago
@gflores-jahia Thank you for your work on this! I convert it to draft: ping me when it will be ready for a final review!
@federicorispo I wonder if you have an opinion regarding the original PR fix of removing ThreadPoolExecutor completely. It looks like this might be a breaking change.
It looks like that we no longer have a default async executor, and users will need to provide one if they want to handle async tasks. But because configuration and HttpInvoker is still created for every request, this to me still doesn't fix the original issue of being able to clean up thread pools. Instead of completely removing the default ThreadPoolExecutor, I wonder if you have ideas on how to properly clean them after execution.
In the meantime will need to put this on hold until we get a good consensus of proper way to fix ThreadPools constantly being created on every http request
@gflores-jahia I will look in the current flow of the request handling to see if there is a clean and solid way to better handle thread after the execution. A good refactor is certainly needed
@gflores-jahia @tdraier
I've made a deep analysis of the codebase and I confirmed the presence of a thread/memory leak in the OsgiGraphQLHttpServlet
. To find a clean solution to this issue, I've investigated the current flow in the GraphQLHttpServlet. When the servlet receives an HTTP request, the doRequest
method is called, which contains the following call chain:
return getConfiguration().getHttpRequestHandler().handle(request, response);
Here's a breakdown of the process:
getConfiguration()
returns the configuration created during servlet initialization, which is shared across all requests and contains a single ThreadPool created implicitly by getAsyncExecutor().getHttpRequestHandler()
returns the HttpRequestHandler, which is created only when the first request is received and then the instance is saved in the configuration for subsequent requests. The creation of the HttpRequestHandler triggers the creation of the HttpRequestInvokerImpl object, passing the same initialized configuration (as before, there is only one instance of this object).handle()
method executes the request and returns a response to the servlet.The main issue with the OsgiGraphQLHttpServlet's behavior is that the getConfiguration method calls OsgiSchemaBuilder#buildConfiguration()
instead of returning an already created configuration instance. This PR introduces:
GraphQLConfigurationProvider
, which binds and unbinds the GraphQLConfiguration
according to the Osgi mechanism.updateConfiguration()
method in the OsgiSchemaBuilder, which updates the configuration without recreating it each time.Now that I have a better understanding of the process, I wouldn't remove the implicit ThreadPool creation because, if the GraphQLConfiguration is correctly created (as in the GraphQLHttpServlet and OsgiGraphQLHttpServlet, thanks to this PR), it will be automatically handled by the GraphQLConfiguration itself. Additionally, the GraphQLConfiguration#with(Executor asyncExecutor)
method allows developers to pass their custom executor.
My only remaining doubt is when the GraphQLConfiguration created by the GraphQLConfigurationProvider is built. I've only seen the with
methods of the Builder, but I haven't seen the build
method that creates the GraphQLConfiguration object: is the GraphQLConfiguration.Builder#build() call delegated to something else?
WORK IN PROGRESS
Creating new PR based off of https://github.com/graphql-java-kickstart/graphql-java-servlet/pull/538 and branched off latest master code.