[GitHub] [flink] zhuzhurk commented on a change in pull request #12181: [FLINK-17645][runtime] Fix SafetyNetCloseableRegistry constructor bug.

2020-05-20 Thread GitBox 


zhuzhurk commented on a change in pull request #12181:
URL: https://github.com/apache/flink/pull/12181#discussion_r427794339



##
File path: 
flink-core/src/test/java/org/apache/flink/core/fs/SafetyNetCloseableRegistryTest.java
##
@@ -198,4 +198,29 @@ public void testReaperThreadSpawnAndStop() throws 
Exception {
}

Assert.assertFalse(SafetyNetCloseableRegistry.isReaperThreadRunning());
}
+
+   /**
+* Test whether failure to start thread in {@link 
SafetyNetCloseableRegistry}
+* constructor can lead to failure of subsequent state check.
+*/
+   @Test
+   public void testReaperThreadStartFailed() throws Exception {
+
+   try {
+   new SafetyNetCloseableRegistry(() -> new 
OutOfMemoryReaperThread());
+   } catch (java.lang.OutOfMemoryError error) {
+   }
+

Review comment:
   We can add `isReaperThreadRunning` checks after the failed creation and 
the succeeded creation. 
   This helps to verify that a reaper thread is really created and running.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [flink] zhuzhurk commented on a change in pull request #12181: [FLINK-17645][runtime] Fix SafetyNetCloseableRegistry constructor bug.

2020-05-20 Thread GitBox 


zhuzhurk commented on a change in pull request #12181:
URL: https://github.com/apache/flink/pull/12181#discussion_r427792440



##
File path: 
flink-core/src/test/java/org/apache/flink/core/fs/SafetyNetCloseableRegistryTest.java
##
@@ -198,4 +198,29 @@ public void testReaperThreadSpawnAndStop() throws 
Exception {
}

Assert.assertFalse(SafetyNetCloseableRegistry.isReaperThreadRunning());
}
+
+   /**
+* Test whether failure to start thread in {@link 
SafetyNetCloseableRegistry}
+* constructor can lead to failure of subsequent state check.
+*/
+   @Test
+   public void testReaperThreadStartFailed() throws Exception {
+
+   try {
+   new SafetyNetCloseableRegistry(() -> new 
OutOfMemoryReaperThread());
+   } catch (java.lang.OutOfMemoryError error) {
+   }
+
+   // the OOM error will not lead to failure of subsequent 
constructor call.
+   SafetyNetCloseableRegistry closeableRegistry = new 
SafetyNetCloseableRegistry();
+   closeableRegistry.close();
+   }
+
+   static class OutOfMemoryReaperThread extends 
SafetyNetCloseableRegistry.CloseableReaperThread {

Review comment:
   can be private





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [flink] zhuzhurk commented on a change in pull request #12181: [FLINK-17645][runtime] Fix SafetyNetCloseableRegistry constructor bug.

2020-05-18 Thread GitBox 


zhuzhurk commented on a change in pull request #12181:
URL: https://github.com/apache/flink/pull/12181#discussion_r427012948



##
File path: 
flink-core/src/main/java/org/apache/flink/core/fs/SafetyNetCloseableRegistry.java
##
@@ -186,6 +212,15 @@ private CloseableReaperThread() {
this.running = true;
}
 
+   @VisibleForTesting
+   CloseableReaperThread(String name) {

Review comment:
   Why adding a new constructor here?
   If you are blocked by the existing private constructor, you can just change 
it to protected.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [flink] zhuzhurk commented on a change in pull request #12181: [FLINK-17645][runtime] Fix SafetyNetCloseableRegistry constructor bug.

2020-05-18 Thread GitBox 


zhuzhurk commented on a change in pull request #12181:
URL: https://github.com/apache/flink/pull/12181#discussion_r427010925



##
File path: 
flink-core/src/main/java/org/apache/flink/core/fs/SafetyNetCloseableRegistry.java
##
@@ -73,8 +73,34 @@
synchronized (REAPER_THREAD_LOCK) {
if (0 == GLOBAL_SAFETY_NET_REGISTRY_COUNT) {
Preconditions.checkState(null == REAPER_THREAD);
-   REAPER_THREAD = new CloseableReaperThread();
-   REAPER_THREAD.start();
+   try {
+   REAPER_THREAD = new 
CloseableReaperThread();
+   REAPER_THREAD.start();
+   } catch (Throwable throwable) {
+   // thread create or start error.
+   REAPER_THREAD = null;
+   throw throwable;
+   }
+   }
+   ++GLOBAL_SAFETY_NET_REGISTRY_COUNT;
+   }
+   }
+
+   @VisibleForTesting
+   SafetyNetCloseableRegistry(CloseableReaperThread reaperThread) {

Review comment:
   We should avoid duplicating the codes.
   Below is a possible way to make it.
   
   ```
SafetyNetCloseableRegistry() {
this(() -> new CloseableReaperThread());
}
   
@VisibleForTesting
SafetyNetCloseableRegistry(Supplier 
reaperThreadSupplier) {
super(new IdentityHashMap<>());
   
synchronized (REAPER_THREAD_LOCK) {
if (0 == GLOBAL_SAFETY_NET_REGISTRY_COUNT) {
Preconditions.checkState(null == REAPER_THREAD);
try {
REAPER_THREAD = 
reaperThreadSupplier.get();
REAPER_THREAD.start();
} catch (Throwable throwable) {
REAPER_THREAD = null;
throw throwable;
}
}
++GLOBAL_SAFETY_NET_REGISTRY_COUNT;
}
}
   ```





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [flink] zhuzhurk commented on a change in pull request #12181: [FLINK-17645][runtime] Fix SafetyNetCloseableRegistry constructor bug.

2020-05-18 Thread GitBox 


zhuzhurk commented on a change in pull request #12181:
URL: https://github.com/apache/flink/pull/12181#discussion_r427010925



##
File path: 
flink-core/src/main/java/org/apache/flink/core/fs/SafetyNetCloseableRegistry.java
##
@@ -73,8 +73,34 @@
synchronized (REAPER_THREAD_LOCK) {
if (0 == GLOBAL_SAFETY_NET_REGISTRY_COUNT) {
Preconditions.checkState(null == REAPER_THREAD);
-   REAPER_THREAD = new CloseableReaperThread();
-   REAPER_THREAD.start();
+   try {
+   REAPER_THREAD = new 
CloseableReaperThread();
+   REAPER_THREAD.start();
+   } catch (Throwable throwable) {
+   // thread create or start error.
+   REAPER_THREAD = null;
+   throw throwable;
+   }
+   }
+   ++GLOBAL_SAFETY_NET_REGISTRY_COUNT;
+   }
+   }
+
+   @VisibleForTesting
+   SafetyNetCloseableRegistry(CloseableReaperThread reaperThread) {

Review comment:
   We should avoid duplicating the codes.
   Below is a possible way to make it.
   
   ```
SafetyNetCloseableRegistry() {
this(() -> new CloseableReaperThread());
}
   
@VisibleForTesting
SafetyNetCloseableRegistry(Supplier 
reaperThreadSupplier) {
super(new IdentityHashMap<>());
   
synchronized (REAPER_THREAD_LOCK) {
if (0 == GLOBAL_SAFETY_NET_REGISTRY_COUNT) {
Preconditions.checkState(null == REAPER_THREAD);
try {
REAPER_THREAD = 
reaperThreadSupplier.get();
REAPER_THREAD.start();
} catch (Throwable throwable) {
// thread create or start error.
REAPER_THREAD = null;
throw throwable;
}
}
++GLOBAL_SAFETY_NET_REGISTRY_COUNT;
}
}
   ```





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [flink] zhuzhurk commented on a change in pull request #12181: [FLINK-17645][runtime] Fix SafetyNetCloseableRegistry constructor bug.

2020-05-17 Thread GitBox 


zhuzhurk commented on a change in pull request #12181:
URL: https://github.com/apache/flink/pull/12181#discussion_r426358510



##
File path: 
flink-core/src/test/java/org/apache/flink/core/fs/SafetyNetCloseableRegistryTest.java
##
@@ -198,4 +211,27 @@ public void testReaperThreadSpawnAndStop() throws 
Exception {
}

Assert.assertFalse(SafetyNetCloseableRegistry.isReaperThreadRunning());
}
+
+   /**
+* Test for FLINK-17645.

Review comment:
   I think this line is not needed. It can be tracked via git log.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [flink] zhuzhurk commented on a change in pull request #12181: [FLINK-17645][runtime] Fix SafetyNetCloseableRegistry constructor bug.

2020-05-17 Thread GitBox 


zhuzhurk commented on a change in pull request #12181:
URL: https://github.com/apache/flink/pull/12181#discussion_r426350896



##
File path: 
flink-core/src/test/java/org/apache/flink/core/fs/SafetyNetCloseableRegistryTest.java
##
@@ -27,18 +27,31 @@
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
 
 import java.io.Closeable;
 import java.io.IOException;
 import java.util.concurrent.atomic.AtomicInteger;
 
+import static org.powermock.api.mockito.PowerMockito.doNothing;
+import static org.powermock.api.mockito.PowerMockito.doThrow;
+import static org.powermock.api.mockito.PowerMockito.whenNew;
+
 /**
  * Tests for the {@link SafetyNetCloseableRegistry}.
  */
+@RunWith(PowerMockRunner.class)

Review comment:
   Mockito is not recommended for Flink tests.
   An alternative is to introduce a new constructor 
`SafetyNetCloseableRegistry(CloseableReaperThread)` and a test class which 
overrides `CloseableReaperThread`.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org