Reusing this thread to note FTR that a new version of POOL is out and that a DBCP PR based of the new POOL is in progress...
Gary On Wed, Sep 23, 2020, 19:28 Gary Gregory <garydgreg...@gmail.com> wrote: > On Wed, Sep 23, 2020 at 6:31 PM Phil Steitz <phil.ste...@gmail.com> wrote: > >> >> 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. >> > > OK, sounds good, I'll push out a [pool] RC later this week after the > [dbcp] RC. > > Gary > > >> >> 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 >> >>
