On Sun, Aug 30, 2020 at 2:30 PM Phil Steitz <phil.ste...@gmail.com> wrote:
> > On 8/30/20 9:22 AM, Gary Gregory wrote: > > 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*) > > Remember this is only called by the pool, so I don't see the need for > custom enums, but it would be good to be able to add enum values so as > you suggested we start with the simplest but then add more granularity > as needed. > Phil, Do you plan on providing this feature? I'm a bit busy with other components and work ATM (aren't we all!) Gary > > Phil > > > 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 > >>>> > >>>> > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > >
