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

Whats the best way to close the connection pool ? #224

Closed
spariev opened this issue Jun 6, 2019 · 7 comments
Closed

Whats the best way to close the connection pool ? #224

spariev opened this issue Jun 6, 2019 · 7 comments
Assignees
Milestone

Comments

@spariev
Copy link

spariev commented Jun 6, 2019

I am deploying an app which uses the carmine on Tomcat and on app server shutdown I see the following warning -

The web application [ROOT##201906061327] appears to have started a thread named [commons-pool-EvictionTimer] but has failed to stop it. This is very likely to create a memory leak.

I looked into carmine code and can't figure out how to close the underlying connection pool, as I cant get a handle on it via the existing API. Can you please advice me how to deal with the problem?

@CmdrDats
Copy link

I have a similar issue - My use case is making sure all threads close themselves cleanly, both for use in CLI tool (clean natural exit without System/exit) and for managing context services (like mysql, kafka, redis, etc) so that I can cleanly restart without resources floating about and without actually restarting the JVM process.

I have tried to (rconn/close-conn (second (rconn/pooled-conn state))) (where state is the {:spec ...}) but it doesn't seem to close it - if I add :pool :none into the config in the first place, that does work correctly, but obviously because it doesn't even create a connection pool in the first place.

What I have found that works is: (.close ^Closeable (second (rconn/pooled-conn state)))) - but it feels a bit tenuous, since I'm not entirely sure there's nothing else floating about? Ideally, I could get the connection pool directly when I first setup the Redis connection and use that instead of the spec map in wcar calls - and then directly close that when I'm done?

@aphyr
Copy link

aphyr commented Mar 3, 2020

I'd like to second this confusion: the fact that Carmine (permanently?) squirrels away connection pools via memoization makes it difficult to tell when and how you can close a connection pool safely, if ever. The memoization thing really worries me, because it feels like you have a choice between uncontrollable sharing of mutable state vs using different :ids and cultivating an ever-growing graveyard of memoized connection pools.

@ptaoussanis
Copy link
Member

ptaoussanis commented Dec 2, 2022

Hi all, apologies for taking forever to reply to this.
Connection pools can be closed with the java.io.Closeable method:

(defonce my-pool (carmine.connections/conn-pool :mem/fresh {})) ; Manually create a pool, bypass cache
(wcar {:pool my-pool} (ping)) ; Use the pool
(.close my-pool) ; Close the pool
(wcar {:pool my-pool} (ping)) ; Will throw, pool is closed

Pool memoization won't be relevant if you're providing your own pool like this.

Apologies for any confusion, the current connections API is poorly documented and can be awkward.
This is an area that's seeing a lot of attention for the next major Carmine version.

Cheers!

@CmdrDats
Copy link

CmdrDats commented Dec 2, 2022

Thanks for the update and all the hard work!

ptaoussanis added a commit that referenced this issue Dec 3, 2022
…onstructor in main ns

Should hopefully help with the confusion re: pooling behaviour.
@ptaoussanis
Copy link
Member

ptaoussanis commented Dec 3, 2022

@CmdrDats You're very welcome!

Quick update: since v4 is still a while away, I've just pushed an intermediate v3.2 release which includes a clarified wcar docstring that should hopefully help with the confusion here.

A pool constructor is now also aliased directly in the core Carmine ns, making it more obvious for folks that want to provide a manually-constructed pool.

Cheers!

@theferrit32
Copy link

@ptaoussanis this is great, thanks for the clarification and update and your work on carmine in general

@spariev
Copy link
Author

spariev commented Dec 5, 2022

@ptaoussanis great to hear! Updated docstring is indeed helpful, thanks for your work on this issue!

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

No branches or pull requests

5 participants