sttp icon indicating copy to clipboard operation
sttp copied to clipboard

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

Open ssalevan opened this issue 3 years ago • 2 comments

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:

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

ssalevan avatar Apr 08 '22 16:04 ssalevan

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.

Pask423 avatar Apr 11 '22 06:04 Pask423

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?

adamw avatar Aug 09 '22 09:08 adamw