Skip to content
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

Fixes for OSGi, remove automatic creation of executor for async requests (WIP) #569

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

gflores-jahia
Copy link
Contributor

WORK IN PROGRESS

Creating new PR based off of #538 and branched off latest master code.

@federicorispo
Copy link
Member

@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 federicorispo self-requested a review May 25, 2024 12:13
@federicorispo federicorispo marked this pull request as draft May 25, 2024 12:14
@gflores-jahia
Copy link
Contributor Author

@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

@federicorispo
Copy link
Member

@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

@federicorispo
Copy link
Member

federicorispo commented Jul 28, 2024

@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:

  • The GraphQLConfigurationProvider, which binds and unbinds the GraphQLConfiguration according to the Osgi mechanism.
  • The 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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants