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

fix(vertx): VertxHttpClientFactory reuses the same Vertx instance for each VertxHttpClient instance #6726

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

manusa
Copy link
Member

@manusa manusa commented Dec 16, 2024

Description

Fixes #6709

Initial proposal to fix the Vertx thread-leaking issue.

  • Switched Vert.x to use daemon threads
  • Refactored VertxHttpClientFactory to re-use the same Vertx instance
  • VertxHttpClientFactory can optionally use a user-provided and user-managed Vertx instance
  • Added shutdown hook to VertxHttpClientFactory to close the default provided Vertx instance

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change
  • Chore (non-breaking change which doesn't affect codebase;
    test, version modification, documentation, etc.)

Checklist

  • Code contributed by me aligns with current project license: Apache 2.0
  • I Added CHANGELOG entry regarding this change
  • I have implemented unit tests to cover my changes
  • I have added/updated the javadocs and other documentation accordingly
  • No new bugs, code smells, etc. in SonarCloud report
  • I tested my code in Kubernetes
  • I tested my code in OpenShift

@manusa manusa added this to the 7.0.1 milestone Dec 16, 2024
@shawkins
Copy link
Contributor

LGTM, I was hoping @vietj could chime in as well as he provided the initial prototype that created an instance per factory.

@manusa
Copy link
Member Author

manusa commented Dec 16, 2024

LGTM, I was hoping @vietj could chime in as well as he provided the initial prototype that created an instance per factory.

Yup, I'd like to get his opinion too.

In any case, I think this fix shouldn't be harmful. The scenario where the factory is created multiple times (which is probably the one the fix impacts most) is leaking at the moment.
I was waiting on your approval to move forward, so unless someone else thinks this should be changed, I think we can merge (I'd really like to get out a release with the fix ASAP).

When we get Julien's comments we can adjust accordingly (it's a bad time in Europe since many have already started winter/Christmas holidays).

@manusa manusa force-pushed the fix/vertx-leak branch 2 times, most recently from 8956de2 to 9fb9ff4 Compare December 16, 2024 17:24
… each VertxHttpClient instance

Signed-off-by: Marc Nuri <marc@marcnuri.com>
@manusa manusa merged commit 03439f2 into fabric8io:main Dec 16, 2024
21 checks passed
@manusa manusa deleted the fix/vertx-leak branch December 16, 2024 20:46
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.

vert.x accepting thread causes memory leak
4 participants