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