Conversation
| try { | ||
| ref = queue.remove(); | ||
| try { | ||
| while(!shutdownLatch.await(100, TimeUnit.MILLISECONDS)) { |
There was a problem hiding this comment.
Do you have a reasoning as to why 100ms?
There was a problem hiding this comment.
Not against a timeout, but 100ms looks way too small.
|
Not really. |
| throw td; | ||
| } catch (Throwable t) { | ||
| t.printStackTrace(); | ||
| } finally { |
There was a problem hiding this comment.
You should release the hatch at the finally block, before setting queue to null:
shutdownLatch.countDown();
|
@jjYBdx4IL I see that you modified the pull request after my comments, just wanted to point out that I'm not a Batik developer nor am I related to the ASF in any way. Thus, my comments do not represent any "official" statement from the Batik developers, the PMC nor the ASF. |
jjYBdx4IL
force-pushed
the
CleanerThreadInterrupt
branch
2 times, most recently
from
8ac91be to
e0f777e
Compare
* de-initialize CleanerThread variables on thread shutdown * don't run CleanerThread as daemon
jjYBdx4IL
force-pushed
the
CleanerThreadInterrupt
branch
from
e0f777e to
1a3a005
Compare
|
Up to Java 18 a workaround existed to stop the thread from outside (eg. application code). This was necessary for re-deploying the app in a JEE environment without having leaks. Now, after Java 21 LTE is mainstream, no more workaround exists (Thread.stop is unsupported). Since this is a real problem for companies and forks exists, may I kindly ask why this won't be included in the original Batik distribution? Can I help getting this integrated? |
I am not a Batik developer but instead the maintainer of one of the forks that you mention: I hope that this PR will not be merged because the patch is incorrect. A BTW my fork has had a correct fix since three years ago :-) |
3 participants
proper fix for:
https://issues.apache.org/jira/browse/BATIK-939
instead of using or suppressing interrupts, a simple CountDownLatch is used to check whether the thread should shut down or not.