[GitHub] [flink] zhuzhurk commented on a change in pull request #12181: [FLINK-17645][runtime] Fix SafetyNetCloseableRegistry constructor bug.
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.
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.
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.
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.
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.
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.
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