On 9/23/20 2:57 PM, Gary Gregory wrote:
This all sounds reasonable but it would be easier to see in a PR.
I will do that once the new version of [pool] is released. Otherwise, I
would have to add snapshot dependency, which I am sure would cause CI to
fail.
Phil
Gary
On Tue, Sep 22, 2020 at 8:30 PM Phil Steitz <phil.ste...@gmail.com> wrote:
On 9/14/20 10:10 AM, Gary Gregory wrote:
On Mon, Sep 14, 2020 at 1:07 PM Phil Steitz <phil.ste...@gmail.com>
wrote:
On 9/14/20 9:36 AM, Gary Gregory wrote:
This feature is now in Pool master. I will prepare an RC soon if you
all think we are good to go so we can then move on to DBCP.
I am still working on testing this in the DBCP use case. Probably best
to wait a little for others to review and for me to get the DBCP change
tested against current pool sources. I should be able to finish that
this weekend.
I implemented changes in DBCP, based on recently committed changes in
pool. I made a few decisions that I would appreciate feedback on.
1. The JDBC abort method requires an Executor as actual parameter. I
added a FixedThreadPool executor with a max of 3 threads to
PoolableConnectionFactory for this purpose. Given that this operation
might hang sometimes, I thought it best to allow more than a single
thread. I guess it could be configurable, but 3 seemed a reasonable
choice. I copied the custom thread factory used by pool's EvictionTimer
to source daemon threads based on ccl for this. I then added a close()
method to PCF that closes the executor and modified BasicDataSource
close to call this.
2. I used p.getObject().getInnermostDelegate().abort(executor) in PCF
destroyObject when abandoned mode is passed in rather than just
getObject().abort as destroy invokes close (I wonder if that is a bug?)
3. I changed JdbcBridge#abort to remove the checkOpen() check. I was
getting exceptions in my concurrency tests where connections were being
closed but then considered abandoned. This is a legit race that abort
will generally handle fine and I don't see the need to throw.
Phil
Sounds good.
Gary
Phil
Gary
On Tue, Sep 8, 2020 at 9:16 AM Gary Gregory <garydgreg...@gmail.com>
wrote:
FWIW, I like the name "DestroyMode" because it matches the "destroy"
in the method name.
Gary
On Mon, Sep 7, 2020, 19:08 Gary Gregory <garydgreg...@gmail.com>
wrote:
On Mon, Sep 7, 2020 at 6:02 PM Phil Steitz <phil.ste...@gmail.com>
wrote:
On 9/3/20 2:44 AM, Mark Thomas wrote:
On 31/08/2020 01:05, Phil Steitz wrote:
<snip/>
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.
Hmm.
On one hand if the driver deadlocks I don't see how that can be
anything
other than a driver bug. If multiple code paths obtain multiple
locks
then those code paths must always obtain those locks in the same
order.
Anything else is a bug that is likely to result in a deadlock in a
multithreaded environment.
On the other hand, it could be argued that the situation only
arises
when an application doesn't correctly return connections to the
pool
and/or keeps them for too long and/or doesn't configure the pool
correctly for their usage pattern.
The approach of adding
PooledObjectFactory.destroyObject(PooledObject,CloseMode)
where CloseMode is an Enum with two values looks reasonable to me.
I have started to work on the [pool] changes for this. I want to
check
two things before completing the PR:
1. "Close" is a [dbcp] concept which does not make sense for all
pool
factories, so I am going to name the enum "DestroyMode" and the two
modes, "Default" and "Abandoned". That leaves room for other modes
like
"Evicted" or "Invalid" later.
2. Speaking of later, technically adding modes will not break binary
compatibility. Are we going to be OK adding outside a major
release?
If the answer is no, I might argue to include the other natural
modes now.
Yes, IMO, it is OK to add enum values in a minor release since it
does
not break binary (or source) compatibility.
Gary
Phil
I do agree that abort() should only be used in the case of
abandoned
connections.
Mark
---------------------------------------------------------------------
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
---------------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
