On 8/30/20 4:00 PM, Gary Gregory wrote:
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!)
If others agree it is a good idea for dbcp, I can do it. I can see the
argument that its better to stay with close() even for abandoned and I
have not been able to get the deadlock to happen, so I would like to
wait a bit and allow others to weigh in. Similarly for pool, I would
like to get more community feedback before adding to the interface.
Phil
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
