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 > >
