Discussion:
[POOL] EvictionTimer daemon thread
(too old to reply)
Wegrzyn, Jakub
2018-01-31 07:11:56 UTC
Permalink
Hi,

We want to upgrade dbcp2 from version 2.1.1 to version 2.2.0. It requires upgrading commons-pool2 from version 2.4.2 to 2.5.0. However, during the upgrade we encountered a problem.
It seems that commons-pool2 Evictor thread has been changed from daemon to non-daemon thread (Issue POOL-351, commit: 4a20cdca923bd342360f821d7020538e985d9ec2).

We cannot find any documentation describing the reason or the change itself. Can you provide more insight why that was changed and add it to the changes-report.

Best regards,
Jakub
Gary Gregory
2018-01-31 07:27:53 UTC
Permalink
I cannot find a POOL-351 issue. Can you double check please?

Gary
Post by Wegrzyn, Jakub
Hi,
We want to upgrade dbcp2 from version 2.1.1 to version 2.2.0. It requires
upgrading commons-pool2 from version 2.4.2 to 2.5.0. However, during the
upgrade we encountered a problem.
It seems that commons-pool2 Evictor thread has been changed from daemon to
non-daemon thread (Issue POOL-351, commit: 4a20cdca923bd342360f821d702053
8e985d9ec2).
We cannot find any documentation describing the reason or the change
itself. Can you provide more insight why that was changed and add it to the
changes-report.
Best regards,
Jakub
Wegrzyn, Jakub
2018-01-31 07:54:06 UTC
Permalink
Bruno P. Kinoshita
2018-01-31 08:15:35 UTC
Permalink
Not sure if it was intentional.

But here's the reason: https://github.com/apache/commons-pool/commit/4a20cdca923bd342360f821d7020538e985d9ec2#diff-38e254894b87bdf9a1758778c7ffd50fL167

Instead of a `new Timer("", /* isDaemon */ true)`, now we have an implementation of `ThreadFactory` that when it creates new `Thread`s, it doesn't set the `setDaemon(true)`. So it just creates a thread with default behaviour of daemon set to false.

As the previous behaviour was to have the threads as daemon, and there doesn't seem to have any arguments for dropping it, we could raise a new issue, with a patch, and ping Mark to see what he thinks?

Cheers
Bruno

(ps: the threads have a typo in their names, but it has been fixed in the master branch already)


________________________________
From: "Wegrzyn, Jakub" <***@sabre.com>
To: Commons Users List <***@commons.apache.org>
Sent: Wednesday, 31 January 2018 8:54 PM
Subject: RE: [POOL] EvictionTimer daemon thread



I couldn’t find it either.
Pool-351 was a commit message.
https://git-wip-us.apache.org/repos/asf?p=commons-pool.git;a=commit;h=4a20cdca923bd342360f821d7020538e985d9ec2

Jakub


-----Original Message-----
From: Gary Gregory [mailto:***@gmail.com]
Sent: Wednesday, January 31, 2018 8:28 AM
To: Commons Users List <***@commons.apache.org>
Subject: Re: [POOL] EvictionTimer daemon thread

I cannot find a POOL-351 issue. Can you double check please?

Gary
Post by Wegrzyn, Jakub
Hi,
We want to upgrade dbcp2 from version 2.1.1 to version 2.2.0. It
requires upgrading commons-pool2 from version 2.4.2 to 2.5.0. However,
during the upgrade we encountered a problem.
It seems that commons-pool2 Evictor thread has been changed from
4a20cdca923bd342360f821d702053 8e985d9ec2).
We cannot find any documentation describing the reason or the change
itself. Can you provide more insight why that was changed and add it
to the changes-report.
Best regards,
Jakub
B�KKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKCB��[��X��ܚX�KK[XZ[�\�\�][��X��ܚX�P��[[ۜ˘\X�K�ܙ�B��܈Y][ۘ[��[X[��K[XZ[�\�\�Z[��[[ۜ˘\X�K�ܙ�B�

---------------------------------------------------------------------
To unsubscribe, e-mail: user-***@commons.apache.org
For additional commands, e-mail: user-***@commons.apache.org
Mark Thomas
2018-01-31 08:49:20 UTC
Permalink
Post by Bruno P. Kinoshita
Not sure if it was intentional.
But here's the reason: https://github.com/apache/commons-pool/commit/4a20cdca923bd342360f821d7020538e985d9ec2#diff-38e254894b87bdf9a1758778c7ffd50fL167
Instead of a `new Timer("", /* isDaemon */ true)`, now we have an implementation of `ThreadFactory` that when it creates new `Thread`s, it doesn't set the `setDaemon(true)`. So it just creates a thread with default behaviour of daemon set to false.
As the previous behaviour was to have the threads as daemon, and there doesn't seem to have any arguments for dropping it, we could raise a new issue, with a patch, and ping Mark to see what he thinks?
There is a typo in the commit message. The issue is POOL-315. It looks
like I had finger trouble that day. I've spotted another typo in the
changelog which I have now fixed.

I don't recall any discussion of whether or not the threads should be
daemon threads.

Starting with a clean slate, I'd turn this around. What is the argument
that the threads should be daemon threads?

If the pool is correctly shutdown it should not matter as the evictor
thread will be stopped at shutdown.

The only reason I can see for changing back to using a daemon thread is
that it used to be a daemon thread and that allowed pools to be used
without shutting them down cleanly.

Overall, I guess I am neutral on changing it.

Mark
Post by Bruno P. Kinoshita
Cheers
Bruno
(ps: the threads have a typo in their names, but it has been fixed in the master branch already)
________________________________
Sent: Wednesday, 31 January 2018 8:54 PM
Subject: RE: [POOL] EvictionTimer daemon thread
I couldn’t find it either.
Pool-351 was a commit message.
https://git-wip-us.apache.org/repos/asf?p=commons-pool.git;a=commit;h=4a20cdca923bd342360f821d7020538e985d9ec2
Jakub
-----Original Message-----
Sent: Wednesday, January 31, 2018 8:28 AM
Subject: Re: [POOL] EvictionTimer daemon thread
I cannot find a POOL-351 issue. Can you double check please?
Gary
Post by Wegrzyn, Jakub
Hi,
We want to upgrade dbcp2 from version 2.1.1 to version 2.2.0. It
requires upgrading commons-pool2 from version 2.4.2 to 2.5.0. However,
during the upgrade we encountered a problem.
It seems that commons-pool2 Evictor thread has been changed from
4a20cdca923bd342360f821d702053 8e985d9ec2).
We cannot find any documentation describing the reason or the change
itself. Can you provide more insight why that was changed and add it
to the changes-report.
Best regards,
Jakub
---------------------------------------------------------------------
---------------------------------------------------------------------
To unsubscribe, e-mail: user-***@commons.apache.org
For additional commands, e-mail: user-***@commons.apache.org
Gary Gregory
2018-01-31 14:38:15 UTC
Permalink
Post by Bruno P. Kinoshita
Post by Bruno P. Kinoshita
Not sure if it was intentional.
But here's the reason: https://github.com/apache/commons-pool/commit/
4a20cdca923bd342360f821d7020538e985d9ec2#diff-
38e254894b87bdf9a1758778c7ffd50fL167
Post by Bruno P. Kinoshita
Instead of a `new Timer("", /* isDaemon */ true)`, now we have an
implementation of `ThreadFactory` that when it creates new `Thread`s, it
doesn't set the `setDaemon(true)`. So it just creates a thread with default
behaviour of daemon set to false.
Post by Bruno P. Kinoshita
As the previous behaviour was to have the threads as daemon, and there
doesn't seem to have any arguments for dropping it, we could raise a new
issue, with a patch, and ping Mark to see what he thinks?
There is a typo in the commit message. The issue is POOL-315. It looks
like I had finger trouble that day. I've spotted another typo in the
changelog which I have now fixed.
I don't recall any discussion of whether or not the threads should be
daemon threads.
Starting with a clean slate, I'd turn this around. What is the argument
that the threads should be daemon threads?
If the pool is correctly shutdown it should not matter as the evictor
thread will be stopped at shutdown.
Are we sure that our shutdown logic always cleans up the thread-pool no
matter what?

Gary
Post by Bruno P. Kinoshita
The only reason I can see for changing back to using a daemon thread is
that it used to be a daemon thread and that allowed pools to be used
without shutting them down cleanly.
Overall, I guess I am neutral on changing it.
Mark
Post by Bruno P. Kinoshita
Cheers
Bruno
(ps: the threads have a typo in their names, but it has been fixed in
the master branch already)
Post by Bruno P. Kinoshita
________________________________
Sent: Wednesday, 31 January 2018 8:54 PM
Subject: RE: [POOL] EvictionTimer daemon thread
I couldn’t find it either.
Pool-351 was a commit message.
https://git-wip-us.apache.org/repos/asf?p=commons-pool.git;a=commit;h=
4a20cdca923bd342360f821d7020538e985d9ec2
Post by Bruno P. Kinoshita
Jakub
-----Original Message-----
Sent: Wednesday, January 31, 2018 8:28 AM
Subject: Re: [POOL] EvictionTimer daemon thread
I cannot find a POOL-351 issue. Can you double check please?
Gary
Post by Wegrzyn, Jakub
Hi,
We want to upgrade dbcp2 from version 2.1.1 to version 2.2.0. It
requires upgrading commons-pool2 from version 2.4.2 to 2.5.0. However,
during the upgrade we encountered a problem.
It seems that commons-pool2 Evictor thread has been changed from
4a20cdca923bd342360f821d702053 8e985d9ec2).
We cannot find any documentation describing the reason or the change
itself. Can you provide more insight why that was changed and add it
to the changes-report.
Best regards,
Jakub
---------------------------------------------------------------------
---------------------------------------------------------------------
Loading...