[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-29 Thread asfgit 
Github user asfgit closed the pull request at:

https://github.com/apache/curator/pull/23


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread dragonsinth 
Github user dragonsinth commented on the pull request:

https://github.com/apache/curator/pull/23#issuecomment-50420280
  
Sweet, looks good.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread Randgalt 
Github user Randgalt commented on the pull request:

https://github.com/apache/curator/pull/23#issuecomment-50419120
  
Sorry Scott - it looks like CURATOR-126 hasn’t sync’d yet to the Github 
repo. The whole Github sync think is a lot of black magic. It is, however, 
here: 
https://git-wip-us.apache.org/repos/asf?p=curator.git;a=tree;h=refs/heads/CURATOR-126;hb=CURATOR-126

From: Scott Blum 
Reply: apache/curator >
Date: July 28, 2014 at 7:03:46 PM
To: apache/curator >
Cc: Jordan Zimmerman >
Subject:  Re: [curator] CURATOR-126: Fix race condition in 
CuratorFrameworkImpl.close() (#23)  

Where are the commits? I did a git remote update on both my fork and apache 
and couldn't find them.

—
Reply to this email directly or view it on GitHub.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread cammckenzie 
Github user cammckenzie commented on the pull request:

https://github.com/apache/curator/pull/23#issuecomment-50419008
  
They're there for me, I just pulled off the apache master (CURATOR-126 
branch). The changes look good to me Jordan, I was trying to avoid changing the 
CuratorFrameworkImpl to add additional debug stuff, but I think it's the way to 
go.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread dragonsinth 
Github user dragonsinth commented on the pull request:

https://github.com/apache/curator/pull/23#issuecomment-50418934
  
Where are the commits?  I did a git remote update on both my fork and 
apache and couldn't find them.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread Randgalt 
Github user Randgalt commented on the pull request:

https://github.com/apache/curator/pull/23#issuecomment-50418758
  
I pushed an update to the test that has an assert. I tested it with the old 
background thread code and the test fails. It succeeds with the new code.

-JZ

From: cammckenzie 
Reply: apache/curator >
Date: July 28, 2014 at 6:29:46 PM
To: apache/curator >
Cc: Jordan Zimmerman >
Subject:  Re: [curator] CURATOR-126: Fix race condition in 
CuratorFrameworkImpl.close() (#23)  

I have pushed a unit test up onto the CURATOR-126 branch. Have a play with 
it, I still seem to be getting the error logged when running against the fix. 
Could be something bogus in the test though.

—
Reply to this email directly or view it on GitHub.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread cammckenzie 
Github user cammckenzie commented on the pull request:

https://github.com/apache/curator/pull/23#issuecomment-50416483
  
I have pushed a unit test up onto the CURATOR-126 branch. Have a play with 
it, I still seem to be getting the error logged when running against the fix. 
Could be something bogus in the test though.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread dragonsinth 
Github user dragonsinth commented on the pull request:

https://github.com/apache/curator/pull/23#issuecomment-50407804
  
That would be great.  If you can send me a commit or diff, I'd be happy to 
play with trying to test it programmatically.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread Randgalt 
Github user Randgalt commented on the pull request:

https://github.com/apache/curator/pull/23#issuecomment-50406296
  
Yeah - please do. Can’t hurt.

From: cammckenzie 
Reply: apache/curator >
Date: July 28, 2014 at 4:41:57 PM
To: apache/curator >
Cc: Jordan Zimmerman >
Subject:  Re: [curator] CURATOR-126: Fix race condition in 
CuratorFrameworkImpl.close() (#23)  

Do you have a unit test to reproduce? I cooked one up which could reproduce 
the issue, but I hadn't found a way for it to work with assertions. I could 
only verify that the exception was being logged. I can still commit it though 
if it's considered of use.

—
Reply to this email directly or view it on GitHub.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread cammckenzie 
Github user cammckenzie commented on the pull request:

https://github.com/apache/curator/pull/23#issuecomment-50406175
  
Do you have a unit test to reproduce? I cooked one up which could reproduce 
the issue, but I hadn't found a way for it to work with assertions. I could 
only verify that the exception was being logged. I can still commit it though 
if it's considered of use.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread Randgalt 
Github user Randgalt commented on a diff in the pull request:

https://github.com/apache/curator/pull/23#discussion_r15491355
  
--- Diff: 
curator-framework/src/main/java/org/apache/curator/framework/CuratorFrameworkFactory.java
 ---
@@ -239,6 +241,16 @@ public Builder connectionTimeoutMs(int 
connectionTimeoutMs)
 }
 
 /**
+ * @param closeWaitMs time to wait during close to join background 
threads
--- End diff --

I think that’s less important as it’s not visible. Up to you.

From: Scott Blum 
Reply: apache/curator >
Date: July 28, 2014 at 4:13:43 PM
To: apache/curator >
Cc: Jordan Zimmerman >
Subject:  Re: [curator] CURATOR-126: Fix race condition in 
CuratorFrameworkImpl.close() (#23)  

In 
curator-framework/src/main/java/org/apache/curator/framework/CuratorFrameworkFactory.java:

> @@ -239,6 +241,16 @@ public Builder connectionTimeoutMs(int 
connectionTimeoutMs)
>  }
>   
>  /**
> + * @param closeWaitMs time to wait during close to join 
background threads
done; what about the constant?

—
Reply to this email directly or view it on GitHub.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread dragonsinth 
Github user dragonsinth commented on a diff in the pull request:

https://github.com/apache/curator/pull/23#discussion_r15491274
  
--- Diff: 
curator-framework/src/main/java/org/apache/curator/framework/CuratorFrameworkFactory.java
 ---
@@ -239,6 +241,16 @@ public Builder connectionTimeoutMs(int 
connectionTimeoutMs)
 }
 
 /**
+ * @param closeWaitMs time to wait during close to join background 
threads
--- End diff --

done; what about the constant?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread Randgalt 
Github user Randgalt commented on a diff in the pull request:

https://github.com/apache/curator/pull/23#discussion_r15490376
  
--- Diff: 
curator-framework/src/main/java/org/apache/curator/framework/CuratorFrameworkFactory.java
 ---
@@ -239,6 +241,16 @@ public Builder connectionTimeoutMs(int 
connectionTimeoutMs)
 }
 
 /**
+ * @param closeWaitMs time to wait during close to join background 
threads
--- End diff --

Sorry to be pedantic, but this would be better named "maxCloseWaitMs". 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread dragonsinth 
Github user dragonsinth commented on a diff in the pull request:

https://github.com/apache/curator/pull/23#discussion_r15490243
  
--- Diff: 
curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java
 ---
@@ -74,7 +71,7 @@
 private final NamespaceFacadeCache namespaceFacadeCache;
 private final NamespaceWatcherMap namespaceWatcherMap = new 
NamespaceWatcherMap(this);
 
-private volatile ExecutorService executorService;
+private volatile Thread backgroundThread;
--- End diff --

Fair point, I'll revert.

BTW: I tried tracing back through the code and was not able to locate the 
code path from ExecutorService.shutdownNow() -> FutureTask.cancel() under 1.6.  
So I might actually be confused and thinking of a Guava executor and not a JDK 
one, it's possible the JDK shutdownNow() is actually safe.  If so, apologizes 
for the mis-info.

Either way, CloseableExecutorService.close() probably does have the issue 
since it does an explicit cancel(true).



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread Randgalt 
Github user Randgalt commented on a diff in the pull request:

https://github.com/apache/curator/pull/23#discussion_r15488051
  
--- Diff: 
curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java
 ---
@@ -74,7 +71,7 @@
 private final NamespaceFacadeCache namespaceFacadeCache;
 private final NamespaceWatcherMap namespaceWatcherMap = new 
NamespaceWatcherMap(this);
 
-private volatile ExecutorService executorService;
+private volatile Thread backgroundThread;
--- End diff --

I'm just concerned it's a larger problem. Executors are used throughout 
Curator and I've relied on shutdownNow to work correctly. I didn't know about 
the bug. So, I'd feel better having an open issue to deal with it directly. 
Curator already has the CloseableExecutorService wrapper and my gut feeling is 
that it can be fixed inside of there.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread Randgalt 
Github user Randgalt commented on a diff in the pull request:

https://github.com/apache/curator/pull/23#discussion_r15487369
  
--- Diff: 
curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java
 ---
@@ -74,7 +71,7 @@
 private final NamespaceFacadeCache namespaceFacadeCache;
 private final NamespaceWatcherMap namespaceWatcherMap = new 
NamespaceWatcherMap(this);
 
-private volatile ExecutorService executorService;
+private volatile Thread backgroundThread;
--- End diff --

But doesn't that mean that Executor.shutdownNow() is broken throughout the 
code? If so, I think there should be a separate issue to fix it globally.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread dragonsinth 
Github user dragonsinth commented on a diff in the pull request:

https://github.com/apache/curator/pull/23#discussion_r15487723
  
--- Diff: 
curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java
 ---
@@ -74,7 +71,7 @@
 private final NamespaceFacadeCache namespaceFacadeCache;
 private final NamespaceWatcherMap namespaceWatcherMap = new 
NamespaceWatcherMap(this);
 
-private volatile ExecutorService executorService;
+private volatile Thread backgroundThread;
--- End diff --

Potentially.  I guess if you feel super strongly I can revert that part of 
the change and just add an awaitTermination.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread dragonsinth 
Github user dragonsinth commented on a diff in the pull request:

https://github.com/apache/curator/pull/23#discussion_r15487087
  
--- Diff: 
curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java
 ---
@@ -300,12 +295,20 @@ public Void apply(CuratorListener listener)
 listeners.clear();
 unhandledErrorListeners.clear();
 connectionStateManager.close();
+if (backgroundThread != null) {
+backgroundThread.interrupt();
+try
+{
+backgroundThread.join(1000);
--- End diff --

Done. I set the default to 1 second, let me know if should be different.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread dragonsinth 
Github user dragonsinth commented on a diff in the pull request:

https://github.com/apache/curator/pull/23#discussion_r15487030
  
--- Diff: 
curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java
 ---
@@ -74,7 +71,7 @@
 private final NamespaceFacadeCache namespaceFacadeCache;
 private final NamespaceWatcherMap namespaceWatcherMap = new 
NamespaceWatcherMap(this);
 
-private volatile ExecutorService executorService;
+private volatile Thread backgroundThread;
--- End diff --

The bug is simple: Executor.shutdownNow() calls FutureTask.cancel(), which 
on older JDK versions has a bug where it can interrupt the wrong thread, 
causing all sorts of problems.  Manually interrupting a thread does not have 
this problem.  Given that this Executor is only every used to run one task (the 
backgroundOperationsLoop) we can simply switch to a Thread and avoid that bug.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread Randgalt 
Github user Randgalt commented on a diff in the pull request:

https://github.com/apache/curator/pull/23#discussion_r15486439
  
--- 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 --

I just checked - that's the only instance of it in the code. If you don't 
mind, please fix it as part of this PR.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread Randgalt 
Github user Randgalt commented on a diff in the pull request:

https://github.com/apache/curator/pull/23#discussion_r15486333
  
--- 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 --

Actually, that's a bug! I didn't see it. It should be 
Thread.currentThread().isInterrupted() which doesn't clear the state. Normally 
I use that one. I'm going to greg the code for other instances of this.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread dragonsinth 
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.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread dragonsinth 
Github user dragonsinth commented on a diff in the pull request:

https://github.com/apache/curator/pull/23#discussion_r15485966
  
--- Diff: 
curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java
 ---
@@ -722,39 +725,35 @@ public void retriesExhausted(OperationAndData 
operationAndData)
 
 private  void 
handleBackgroundOperationException(OperationAndData 
operationAndData, Throwable e)
 {
-do
+if ( (operationAndData != null) && RetryLoop.isRetryException(e) )
 {
-if ( (operationAndData != null) && 
RetryLoop.isRetryException(e) )
+if ( 
!Boolean.getBoolean(DebugUtils.PROPERTY_DONT_LOG_CONNECTION_ISSUES) )
+{
+log.debug("Retry-able exception received", e);
+}
+if ( 
client.getRetryPolicy().allowRetry(operationAndData.getThenIncrementRetryCount(),
 operationAndData.getElapsedTimeMs(), operationAndData) )
 {
 if ( 
!Boolean.getBoolean(DebugUtils.PROPERTY_DONT_LOG_CONNECTION_ISSUES) )
 {
-log.debug("Retry-able exception received", e);
+log.debug("Retrying operation");
 }
-if ( 
client.getRetryPolicy().allowRetry(operationAndData.getThenIncrementRetryCount(),
 operationAndData.getElapsedTimeMs(), operationAndData) )
+backgroundOperations.offer(operationAndData);
+return;
+}
+else
+{
+if ( 
!Boolean.getBoolean(DebugUtils.PROPERTY_DONT_LOG_CONNECTION_ISSUES) )
 {
-if ( 
!Boolean.getBoolean(DebugUtils.PROPERTY_DONT_LOG_CONNECTION_ISSUES) )
-{
-log.debug("Retrying operation");
-}
-backgroundOperations.offer(operationAndData);
-break;
+log.debug("Retry policy did not allow retry");
 }
-else
+if ( operationAndData.getErrorCallback() != null )
 {
-if ( 
!Boolean.getBoolean(DebugUtils.PROPERTY_DONT_LOG_CONNECTION_ISSUES) )
-{
-log.debug("Retry policy did not allow retry");
-}
-if ( operationAndData.getErrorCallback() != null )
-{
-
operationAndData.getErrorCallback().retriesExhausted(operationAndData);
-}
+
operationAndData.getErrorCallback().retriesExhausted(operationAndData);
 }
 }
-
-logError("Background exception was not retry-able or retry 
gave up", e);
 }
-while ( false );
+
+logError("Background exception was not retry-able or retry gave 
up", e);
--- End diff --

Sure thing.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread Randgalt 
Github user Randgalt commented on a diff in the pull request:

https://github.com/apache/curator/pull/23#discussion_r15485833
  
--- Diff: 
curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java
 ---
@@ -74,7 +71,7 @@
 private final NamespaceFacadeCache namespaceFacadeCache;
 private final NamespaceWatcherMap namespaceWatcherMap = new 
NamespaceWatcherMap(this);
 
-private volatile ExecutorService executorService;
+private volatile Thread backgroundThread;
--- End diff --

I disagree with changing this to Thread. I understand that there are some 
bugs with Executor but that's a much larger issues. Curator uses Executor 
everywhere. If the Executor bug will exhibit here it will do so everywhere. We 
can consider a global fix for the bug (which I don't yet understand).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread Randgalt 
Github user Randgalt commented on a diff in the pull request:

https://github.com/apache/curator/pull/23#discussion_r15485742
  
--- 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 --

This is not correct. _Always_ re-set the interrupted state of threads. The 
break statement exists the while condition.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread Randgalt 
Github user Randgalt commented on a diff in the pull request:

https://github.com/apache/curator/pull/23#discussion_r15485690
  
--- 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 --

Threads should always be re-interrupted. It may be that a break statement 
should be added.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread Randgalt 
Github user Randgalt commented on a diff in the pull request:

https://github.com/apache/curator/pull/23#discussion_r15485603
  
--- Diff: 
curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java
 ---
@@ -722,39 +725,35 @@ public void retriesExhausted(OperationAndData 
operationAndData)
 
 private  void 
handleBackgroundOperationException(OperationAndData 
operationAndData, Throwable e)
 {
-do
+if ( (operationAndData != null) && RetryLoop.isRetryException(e) )
 {
-if ( (operationAndData != null) && 
RetryLoop.isRetryException(e) )
+if ( 
!Boolean.getBoolean(DebugUtils.PROPERTY_DONT_LOG_CONNECTION_ISSUES) )
+{
+log.debug("Retry-able exception received", e);
+}
+if ( 
client.getRetryPolicy().allowRetry(operationAndData.getThenIncrementRetryCount(),
 operationAndData.getElapsedTimeMs(), operationAndData) )
 {
 if ( 
!Boolean.getBoolean(DebugUtils.PROPERTY_DONT_LOG_CONNECTION_ISSUES) )
 {
-log.debug("Retry-able exception received", e);
+log.debug("Retrying operation");
 }
-if ( 
client.getRetryPolicy().allowRetry(operationAndData.getThenIncrementRetryCount(),
 operationAndData.getElapsedTimeMs(), operationAndData) )
+backgroundOperations.offer(operationAndData);
+return;
+}
+else
+{
+if ( 
!Boolean.getBoolean(DebugUtils.PROPERTY_DONT_LOG_CONNECTION_ISSUES) )
 {
-if ( 
!Boolean.getBoolean(DebugUtils.PROPERTY_DONT_LOG_CONNECTION_ISSUES) )
-{
-log.debug("Retrying operation");
-}
-backgroundOperations.offer(operationAndData);
-break;
+log.debug("Retry policy did not allow retry");
 }
-else
+if ( operationAndData.getErrorCallback() != null )
 {
-if ( 
!Boolean.getBoolean(DebugUtils.PROPERTY_DONT_LOG_CONNECTION_ISSUES) )
-{
-log.debug("Retry policy did not allow retry");
-}
-if ( operationAndData.getErrorCallback() != null )
-{
-
operationAndData.getErrorCallback().retriesExhausted(operationAndData);
-}
+
operationAndData.getErrorCallback().retriesExhausted(operationAndData);
 }
 }
-
-logError("Background exception was not retry-able or retry 
gave up", e);
 }
-while ( false );
+
+logError("Background exception was not retry-able or retry gave 
up", e);
--- End diff --

Yes, please revert. Create a separate PR if you like.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread Randgalt 
Github user Randgalt commented on a diff in the pull request:

https://github.com/apache/curator/pull/23#discussion_r15485565
  
--- Diff: 
curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java
 ---
@@ -300,12 +295,20 @@ public Void apply(CuratorListener listener)
 listeners.clear();
 unhandledErrorListeners.clear();
 connectionStateManager.close();
+if (backgroundThread != null) {
+backgroundThread.interrupt();
+try
+{
+backgroundThread.join(1000);
--- End diff --

Add a field to the CuratorFrameworkFactory builder so that this value can 
be configured.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread dragonsinth 
Github user dragonsinth commented on a diff in the pull request:

https://github.com/apache/curator/pull/23#discussion_r15483300
  
--- Diff: 
curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java
 ---
@@ -300,12 +295,20 @@ public Void apply(CuratorListener listener)
 listeners.clear();
 unhandledErrorListeners.clear();
 connectionStateManager.close();
+if (backgroundThread != null) {
+backgroundThread.interrupt();
+try
+{
+backgroundThread.join(1000);
--- End diff --

I totally made this number up.  What's the right thing here?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread dragonsinth 
Github user dragonsinth commented on a diff in the pull request:

https://github.com/apache/curator/pull/23#discussion_r15483196
  
--- 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 --

not necessary; also inconsistent with the while condition.  If the loop 
exits because the while condition test reads the interrupted value, it's not 
resetting the interrupted flag anyway, so there's no point doing it on this 
exit path either.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread dragonsinth 
Github user dragonsinth commented on a diff in the pull request:

https://github.com/apache/curator/pull/23#discussion_r15483096
  
--- Diff: 
curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java
 ---
@@ -722,39 +725,35 @@ public void retriesExhausted(OperationAndData 
operationAndData)
 
 private  void 
handleBackgroundOperationException(OperationAndData 
operationAndData, Throwable e)
 {
-do
+if ( (operationAndData != null) && RetryLoop.isRetryException(e) )
 {
-if ( (operationAndData != null) && 
RetryLoop.isRetryException(e) )
+if ( 
!Boolean.getBoolean(DebugUtils.PROPERTY_DONT_LOG_CONNECTION_ISSUES) )
+{
+log.debug("Retry-able exception received", e);
+}
+if ( 
client.getRetryPolicy().allowRetry(operationAndData.getThenIncrementRetryCount(),
 operationAndData.getElapsedTimeMs(), operationAndData) )
 {
 if ( 
!Boolean.getBoolean(DebugUtils.PROPERTY_DONT_LOG_CONNECTION_ISSUES) )
 {
-log.debug("Retry-able exception received", e);
+log.debug("Retrying operation");
 }
-if ( 
client.getRetryPolicy().allowRetry(operationAndData.getThenIncrementRetryCount(),
 operationAndData.getElapsedTimeMs(), operationAndData) )
+backgroundOperations.offer(operationAndData);
+return;
+}
+else
+{
+if ( 
!Boolean.getBoolean(DebugUtils.PROPERTY_DONT_LOG_CONNECTION_ISSUES) )
 {
-if ( 
!Boolean.getBoolean(DebugUtils.PROPERTY_DONT_LOG_CONNECTION_ISSUES) )
-{
-log.debug("Retrying operation");
-}
-backgroundOperations.offer(operationAndData);
-break;
+log.debug("Retry policy did not allow retry");
 }
-else
+if ( operationAndData.getErrorCallback() != null )
 {
-if ( 
!Boolean.getBoolean(DebugUtils.PROPERTY_DONT_LOG_CONNECTION_ISSUES) )
-{
-log.debug("Retry policy did not allow retry");
-}
-if ( operationAndData.getErrorCallback() != null )
-{
-
operationAndData.getErrorCallback().retriesExhausted(operationAndData);
-}
+
operationAndData.getErrorCallback().retriesExhausted(operationAndData);
 }
 }
-
-logError("Background exception was not retry-able or retry 
gave up", e);
 }
-while ( false );
+
+logError("Background exception was not retry-able or retry gave 
up", e);
--- End diff --

This is unrelated to the main change.  While I was in here, I noticed this 
method was more complicated than it needed to be.  I could revert this section 
if you guys would prefer to isolate changes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-126: Fix race condition in CuratorFr...

2014-07-28 Thread dragonsinth 
GitHub user dragonsinth opened a pull request:

https://github.com/apache/curator/pull/23

CURATOR-126: Fix race condition in CuratorFrameworkImpl.close()



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/dragonsinth/curator CURATOR-126

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/curator/pull/23.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #23


commit 247e02152a96b58f344439d589d02065f1bd2632
Author: Scott Blum 
Date:   2014-07-28T18:10:37Z

CURATOR-126: Fix race condition in CuratorFrameworkImpl.close()




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---