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

[sttp-finagle] Adds concurrent cache to prevent Finagle client leak #1393

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ssalevan
Copy link

@ssalevan ssalevan commented Apr 8, 2022

sttp-finagle creates a new Finagle HTTP client upon each STTP request, causing a client leak that will result in eventual GC and resolution issues for dests that reflect large clusters.

This pull request adds a ConcurrentHashMap[String, Service[http.Request, FResponse]] to sttp-finagle, keyed upon the Finagle dest. This change will pair a single Finagle client to each backend dest, resolving this issue. I've also added a .jvmopts file to the root of this repository to address some OOM issues I ran into during the execution of sbt test.

Let me know what you think and thanks very much for the consideration!

Before submitting pull request:

  • Check if the project compiles by running sbt compile
  • Verify docs compilation by running sbt compileDocs
  • Check if tests pass by running sbt test
  • Format code by running sbt scalafmt

@Pask423
Copy link
Contributor

Pask423 commented Apr 11, 2022

Could you please elaborate more on the problem as I am not sure what it exactly is.
I am not sure but GC here should not be a problem as I think that client is still referenced so it should not be collected.

.jvmopts Outdated
@@ -0,0 +1,4 @@
-Xms512M
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this config should be removed as it interfere with our CI JVM settings.
To solve problems with OOME in tests I would advise to change sbt memory settings, maybe sbt -mem 4096 is enought

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I've removed this file.

@adamw
Copy link
Member

adamw commented Aug 9, 2022

I think if we wanted such a cache we would need to expose it somehow so that it can be cleared, or at least allow the user to provide a cache implementation. Alternatively the finagle clients could be closed after each request (if a non-standard timeout is given).

Shouldn't the clients be cached by the timeout in the first place? Why are the new clients created, is it always per-host or per-settings?

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.

3 participants