[
https://issues.apache.org/jira/browse/CURATOR-126?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14076709#comment-14076709
]
ASF GitHub Bot commented on CURATOR-126:
----------------------------------------
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/23#discussion_r15486219
--- Diff:
curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java
---
@@ -770,9 +769,8 @@ private void backgroundOperationsLoop()
debugListener.listen(operationAndData);
}
}
- catch ( InterruptedException e )
+ catch ( InterruptedException ignored )
{
- Thread.currentThread().interrupt();
--- End diff --
Let me be more clear. The way the loop is constructed:
```
private void backgroundOperationsLoop()
{
while ( !Thread.interrupted() )
{ ... }
```
ALREADY eats the interrupted status. Simply checking
`Thread.interrupted()` consumes it. If you want to consistently enforce a rule
that you always re-interrupt threads (which is a good rule in general, although
not necessary here) then you need an unconditional re-interrupt at the end of
the method.
Do you want me to add that?
My point is that putting the interrupt only in the catch block is
inconsistent. It re-interrupts in the case where an InterruptedException gets
throws, and fails to re-interrupt when the loop exits without exception.
> IllegalStateException in performBackgroundOperation during close
> ----------------------------------------------------------------
>
> Key: CURATOR-126
> URL: https://issues.apache.org/jira/browse/CURATOR-126
> Project: Apache Curator
> Issue Type: Bug
> Components: Framework
> Affects Versions: 2.5.0
> Reporter: Scott Blum
> Assignee: Cameron McKenzie
> Original Estimate: 24h
> Remaining Estimate: 24h
>
> {code}
> [CuratorFramework-0] ERROR
> org.apache.curator.framework.imps.CuratorFrameworkImpl - Background
> exception was not retry-able or retry gave up
> java.lang.IllegalStateException: Client is not started
> at
> com.google.common.base.Preconditions.checkState(Preconditions.java:176)
> at
> org.apache.curator.CuratorZookeeperClient.getZooKeeper(CuratorZookeeperClient.java:113)
> at
> org.apache.curator.framework.imps.CuratorFrameworkImpl.performBackgroundOperation(CuratorFrameworkImpl.java:807)
> at
> org.apache.curator.framework.imps.CuratorFrameworkImpl.backgroundOperationsLoop(CuratorFrameworkImpl.java:793)
> at
> org.apache.curator.framework.imps.CuratorFrameworkImpl.access$400(CuratorFrameworkImpl.java:57)
> at
> org.apache.curator.framework.imps.CuratorFrameworkImpl$4.call(CuratorFrameworkImpl.java:275)
> at java.util.concurrent.FutureTask.run(FutureTask.java:266)
> at
> java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
> at
> java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
> at java.lang.Thread.run(Thread.java:744)
> {code}
> I see this sometimes during test runs; I believe this happens because
> CuratorZookeeperClient.started gets set to false during shutdown, but the
> backgroundOperation loop can still be running since shutting down the
> backgroundOperation loop is inherently racy.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
