Hm... would we need the flexibility of passing custom enums? For example, CloseMode could be an interface implemented by various enums in the style of the JRE's NIO public enum StandardOpenOption implements OpenOption? We could have: PooledObjectFactory.destroyObject(PooledObject, DestroyOption*) Where DestroyOption is an empty interface and we provide a StandardDestroyOption enum that implements DestroyOption. ? Gary
On Sun, Aug 30, 2020 at 11:49 AM Gary Gregory <garydgreg...@gmail.com> wrote: > So concretely, we would > add org.apache.commons.pool2.PooledObjectFactory.destroyObject(PooledObject, > CloseMode*) as a default method. > [*] New enum > > ? > > Gary > > > On Sat, Aug 29, 2020 at 4:02 PM Gary Gregory <garydgreg...@gmail.com> > wrote: > >> On Sat, Aug 29, 2020 at 2:41 PM Phil Steitz <phil.ste...@gmail.com> >> wrote: >> >>> >>> On 8/29/20 11:03 AM, Gary Gregory wrote: >>> > On Sat, Aug 29, 2020 at 1:35 PM Phil Steitz <phil.ste...@gmail.com> >>> wrote: >>> > >>> >> A pool-related deadlock was reported recently in [1] to tomcat-user. >>> >> The OP was using a different pool, but it looks to me like the same >>> >> deadlock could happen with dbcp. The source is arguably a driver bug, >>> >> but in [2], the driver maintainer makes the good point that to avoid >>> the >>> >> problem in [1] (and others like it) it would be better if pool >>> >> maintainers used abort instead of close to disconnect "stuck" or >>> >> abandoned connections. That makes sense to me. >>> >> >>> >> To implement this in dbcp, we would need to either always use abort >>> >> instead of close when closing physical connections (a bad idea, IMO, >>> as >>> >> it could mess up counters due to async execution and add overhead) >>> > >>> > I agree that changing the happy path from close() to abort() is a bad >>> idea; >>> > especially since the Connection#close() Javadoc contract releases >>> database >>> > resources while abort() does not or is vague about it (at least in the >>> Java >>> > 8 Javadoc). >>> >>> Yes, bad idea to do that, I think. >>> >>> > >>> > >>> >> or >>> >> modify pool to call a different factory method than destroy when >>> >> triggering close-abandoned. I think the latter makes sense and it >>> could >>> >> be useful for other pool clients to be able to this kind of thing as >>> well. >>> >> >>> > This means that we would treat abandoned connections like a real >>> resource >>> > leak in the sense that (if) abort() leaks database resources, then the >>> DBA >>> > would have to deal with that. >>> >>> Yeah, unless the driver does it cleanly. The other thing to note here >>> is that abort is async and is supposed to allow ops in progress to >>> complete, etc. That is kinder to clients but technically allows >>> maxActive to exceed the cap while the abort is in progress. >>> >>> > >>> > I think formally surfacing an abort path vs. the current close/destroy >>> is >>> > OK since it reflects what JDBC allows. If that means trickling the >>> concept >>> > down to Pool, then that's OK as well IMO. This could be generically >>> > surfaced with a "close mode" enum passed to close/destroy APIs. This is >>> > similar to what we did in HttpCore's CloseMode(IMMEDIATE vs GRACEFUL): >>> > >>> https://javadoc.io/static/org.apache.httpcomponents.core5/httpcore5/5.0.1/org/apache/hc/core5/io/CloseMode.html >>> >>> Exactly. I think the abandoned-destroy is legitimately different from >>> the other destroy use cases and I could see other kinds of pools >>> (sockets, jms, etc) benefiting from having the ability to handle destroy >>> differently for abandoned (or other) cases. >>> >>> > >>> > >>> >> What I propose is that we make these change to pool: >>> >> >>> >> 1. Add a new method to PooledObjectFactory called "abandonObject" >>> with >>> >> default implementation delegating to destroyObject. >>> >> >>> > That or overload destroyObject with a destroyObject(CloseMode) would be >>> > easier to find. >>> >>> Yes, I think that would be better. That raises the interesting question >>> of what are the CloseModes. All we need for this issue is normal vs >>> abandoned, but I can see value in providing fuller context to >>> factories. So if we are doing it, why not something like: >>> >>> invalidated >>> abandoned >>> evicted >>> excess idle >>> failed validation >>> failed activation >>> failed passivation >>> clearing pool >>> >> >> I think a YAGNI-style approach would work well here by starting with 2 >> modes: one to support the current behavior (default) and another to support >> the new behavior backed by Connection#abort() in DBCP. >> A third "escape hatch" mode could see passing in a lamba I suppose. I >> really think we should start by just focusing on the one issue at hand >> before we enlarge the solution. >> >> 2c, >> Gary >> >> >>> Phil >>> >>> >>> >>> > >>> > Gary >>> > >>> > >>> >> 2. Change the logic in removeAbandoned to call the new factory >>> method >>> >> instead of destroyObject >>> >> >>> >> And then in dbcp: >>> >> >>> >> * Add implementation of abandonObject in PoolableConnectionFactory >>> to >>> >> call abort. >>> >> >>> >> wdyt? >>> >> >>> >> Phil >>> >> >>> >> [1] >>> >> >>> >> >>> https://lists.apache.org/thread.html/r7095d641bbc8db0a526d2d9a18684202347a747cf0e315a4a50d2001%40%3Cusers.tomcat.apache.org%3E >>> >> >>> >> [2] https://bugs.mysql.com/bug.php?id=64954 >>> >> >>> >> >>> >> >>> >>> --------------------------------------------------------------------- >>> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >>> For additional commands, e-mail: dev-h...@commons.apache.org >>> >>>
