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