This all sounds reasonable but it would be easier to see in a PR. 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 > >
