[jira] [Commented] (GEODE-2109) calling submit on ExecutionService can cause exceptions to be lost
[ https://issues.apache.org/jira/browse/GEODE-2109?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15726902#comment-15726902 ] ASF GitHub Bot commented on GEODE-2109: --- Github user upthewaterspout commented on the issue: https://github.com/apache/geode/pull/296 +1 looks good to me. > calling submit on ExecutionService can cause exceptions to be lost > -- > > Key: GEODE-2109 > URL: https://issues.apache.org/jira/browse/GEODE-2109 > Project: Geode > Issue Type: Bug > Components: regions >Reporter: Darrel Schneider > > Geode has a number of places that call submit on ExecutionService. The submit > method returns a Future object. If the caller makes sure it calls "get" on > the Future then all is well. But in many places geode is not calling get. In > that case if the Runnable that was submitted throws an exception it gets > stored in the get and never logged. This can make it very hard to diagnose > problems. > If the caller does not want to call get on the returned Future then it should > instead call the "execute" method. In that case the exception will be > unhandled and the unhandled exception handler code we have on the > LoggingThreadGroup class will cause the exception to be logged. > Here are the places that should be changed to use execute instead of submit: > org.apache.geode.internal.util.concurrent.CustomEntryConcurrentHashMap.clear() > org.apache.geode.internal.cache.DiskStoreImpl.executeDiskStoreTask(Runnable) > org.apache.geode.internal.cache.lru.HeapEvictor.onEvent(MemoryEvent) > org.apache.geode.distributed.internal.InternalLocator.restartWithDS(InternalDistributedSystem, > GemFireCacheImpl) > org.apache.geode.distributed.internal.FunctionExecutionPooledExecutor.FunctionExecutionPooledExecutor(BlockingQueue, > int, PoolStatHelper, ThreadFactory, int, boolean) > org.apache.geode.internal.cache.PRHARedundancyProvider.scheduleCreateMissingBuckets() > org.apache.geode.distributed.internal.InternalLocator.startSharedConfigurationService(GemFireCacheImpl) > org.apache.geode.cache.client.internal.SingleHopClientExecutor.submitTask(Runnable) > org.apache.geode.management.internal.FederatingManager.submitTask(Callable) -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (GEODE-2109) calling submit on ExecutionService can cause exceptions to be lost
[ https://issues.apache.org/jira/browse/GEODE-2109?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15726438#comment-15726438 ] ASF GitHub Bot commented on GEODE-2109: --- Github user kirklund commented on the issue: https://github.com/apache/geode/pull/296 I think this is ready to commit. I'll check with @upthewaterspout. Thanks! > calling submit on ExecutionService can cause exceptions to be lost > -- > > Key: GEODE-2109 > URL: https://issues.apache.org/jira/browse/GEODE-2109 > Project: Geode > Issue Type: Bug > Components: regions >Reporter: Darrel Schneider > > Geode has a number of places that call submit on ExecutionService. The submit > method returns a Future object. If the caller makes sure it calls "get" on > the Future then all is well. But in many places geode is not calling get. In > that case if the Runnable that was submitted throws an exception it gets > stored in the get and never logged. This can make it very hard to diagnose > problems. > If the caller does not want to call get on the returned Future then it should > instead call the "execute" method. In that case the exception will be > unhandled and the unhandled exception handler code we have on the > LoggingThreadGroup class will cause the exception to be logged. > Here are the places that should be changed to use execute instead of submit: > org.apache.geode.internal.util.concurrent.CustomEntryConcurrentHashMap.clear() > org.apache.geode.internal.cache.DiskStoreImpl.executeDiskStoreTask(Runnable) > org.apache.geode.internal.cache.lru.HeapEvictor.onEvent(MemoryEvent) > org.apache.geode.distributed.internal.InternalLocator.restartWithDS(InternalDistributedSystem, > GemFireCacheImpl) > org.apache.geode.distributed.internal.FunctionExecutionPooledExecutor.FunctionExecutionPooledExecutor(BlockingQueue, > int, PoolStatHelper, ThreadFactory, int, boolean) > org.apache.geode.internal.cache.PRHARedundancyProvider.scheduleCreateMissingBuckets() > org.apache.geode.distributed.internal.InternalLocator.startSharedConfigurationService(GemFireCacheImpl) > org.apache.geode.cache.client.internal.SingleHopClientExecutor.submitTask(Runnable) > org.apache.geode.management.internal.FederatingManager.submitTask(Callable) -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (GEODE-2109) calling submit on ExecutionService can cause exceptions to be lost
[ https://issues.apache.org/jira/browse/GEODE-2109?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15720032#comment-15720032 ] ASF GitHub Bot commented on GEODE-2109: --- Github user deepakddixit commented on the issue: https://github.com/apache/incubator-geode/pull/296 @kirklund I have added a test as suggested with SingleHopTestExecutor. Kindly review. > calling submit on ExecutionService can cause exceptions to be lost > -- > > Key: GEODE-2109 > URL: https://issues.apache.org/jira/browse/GEODE-2109 > Project: Geode > Issue Type: Bug > Components: regions >Reporter: Darrel Schneider > > Geode has a number of places that call submit on ExecutionService. The submit > method returns a Future object. If the caller makes sure it calls "get" on > the Future then all is well. But in many places geode is not calling get. In > that case if the Runnable that was submitted throws an exception it gets > stored in the get and never logged. This can make it very hard to diagnose > problems. > If the caller does not want to call get on the returned Future then it should > instead call the "execute" method. In that case the exception will be > unhandled and the unhandled exception handler code we have on the > LoggingThreadGroup class will cause the exception to be logged. > Here are the places that should be changed to use execute instead of submit: > org.apache.geode.internal.util.concurrent.CustomEntryConcurrentHashMap.clear() > org.apache.geode.internal.cache.DiskStoreImpl.executeDiskStoreTask(Runnable) > org.apache.geode.internal.cache.lru.HeapEvictor.onEvent(MemoryEvent) > org.apache.geode.distributed.internal.InternalLocator.restartWithDS(InternalDistributedSystem, > GemFireCacheImpl) > org.apache.geode.distributed.internal.FunctionExecutionPooledExecutor.FunctionExecutionPooledExecutor(BlockingQueue, > int, PoolStatHelper, ThreadFactory, int, boolean) > org.apache.geode.internal.cache.PRHARedundancyProvider.scheduleCreateMissingBuckets() > org.apache.geode.distributed.internal.InternalLocator.startSharedConfigurationService(GemFireCacheImpl) > org.apache.geode.cache.client.internal.SingleHopClientExecutor.submitTask(Runnable) > org.apache.geode.management.internal.FederatingManager.submitTask(Callable) -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (GEODE-2109) calling submit on ExecutionService can cause exceptions to be lost
[
https://issues.apache.org/jira/browse/GEODE-2109?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15712628#comment-15712628
]
ASF GitHub Bot commented on GEODE-2109:
---
Github user kirklund commented on the issue:
https://github.com/apache/incubator-geode/pull/296
We've been requiring unit tests or integration tests for all bug fixes.
At a minimum, I would recommend adding a new test that fails due to at
least of the conditions reported by GEODE-2109 and then passes with your
changes. Example:
`
import static org.assertj.core.api.Assertions.assertThat;
import org.apache.geode.test.junit.categories.UnitTest;
import org.junit.Rule;
import org.junit.Test;
import org.junit.contrib.java.lang.system.SystemErrRule;
import org.junit.experimental.categories.Category;
@Category(UnitTest.class)
public class FederatingManagerSubmitTaskWithFailureTest {
private FederatingManager manager;
@Rule
public SystemErrRule systemErrRule = new SystemErrRule().enableLog();
@Test
public void submittedTaskShouldLogFailure() throws Exception {
manager.submitTask(() -> {throw new Exception("error message");});
assertThat(systemErrRule.getLog()).contains(Exception.class.getName()).contains("error
message");
}
}
`
The above test would require a couple things:
1) some sort of @Before method which instantiates manager
2) change FederatingManager.submitTask(...) to package scope so the unit
test can call it (right now FederatingManager is not a test-friendly class)
> calling submit on ExecutionService can cause exceptions to be lost
> --
>
> Key: GEODE-2109
> URL: https://issues.apache.org/jira/browse/GEODE-2109
> Project: Geode
> Issue Type: Bug
> Components: regions
>Reporter: Darrel Schneider
>
> Geode has a number of places that call submit on ExecutionService. The submit
> method returns a Future object. If the caller makes sure it calls "get" on
> the Future then all is well. But in many places geode is not calling get. In
> that case if the Runnable that was submitted throws an exception it gets
> stored in the get and never logged. This can make it very hard to diagnose
> problems.
> If the caller does not want to call get on the returned Future then it should
> instead call the "execute" method. In that case the exception will be
> unhandled and the unhandled exception handler code we have on the
> LoggingThreadGroup class will cause the exception to be logged.
> Here are the places that should be changed to use execute instead of submit:
> org.apache.geode.internal.util.concurrent.CustomEntryConcurrentHashMap.clear()
> org.apache.geode.internal.cache.DiskStoreImpl.executeDiskStoreTask(Runnable)
> org.apache.geode.internal.cache.lru.HeapEvictor.onEvent(MemoryEvent)
> org.apache.geode.distributed.internal.InternalLocator.restartWithDS(InternalDistributedSystem,
> GemFireCacheImpl)
> org.apache.geode.distributed.internal.FunctionExecutionPooledExecutor.FunctionExecutionPooledExecutor(BlockingQueue,
> int, PoolStatHelper, ThreadFactory, int, boolean)
> org.apache.geode.internal.cache.PRHARedundancyProvider.scheduleCreateMissingBuckets()
> org.apache.geode.distributed.internal.InternalLocator.startSharedConfigurationService(GemFireCacheImpl)
> org.apache.geode.cache.client.internal.SingleHopClientExecutor.submitTask(Runnable)
> org.apache.geode.management.internal.FederatingManager.submitTask(Callable)
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (GEODE-2109) calling submit on ExecutionService can cause exceptions to be lost
[ https://issues.apache.org/jira/browse/GEODE-2109?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15709145#comment-15709145 ] ASF GitHub Bot commented on GEODE-2109: --- Github user deepakddixit commented on the issue: https://github.com/apache/incubator-geode/pull/296 @upthewaterspout Thanks for the review. The class declaration is mistakenly changed. I have corrected it. All tests are passing from precheckin after this. > calling submit on ExecutionService can cause exceptions to be lost > -- > > Key: GEODE-2109 > URL: https://issues.apache.org/jira/browse/GEODE-2109 > Project: Geode > Issue Type: Bug > Components: regions >Reporter: Darrel Schneider > > Geode has a number of places that call submit on ExecutionService. The submit > method returns a Future object. If the caller makes sure it calls "get" on > the Future then all is well. But in many places geode is not calling get. In > that case if the Runnable that was submitted throws an exception it gets > stored in the get and never logged. This can make it very hard to diagnose > problems. > If the caller does not want to call get on the returned Future then it should > instead call the "execute" method. In that case the exception will be > unhandled and the unhandled exception handler code we have on the > LoggingThreadGroup class will cause the exception to be logged. > Here are the places that should be changed to use execute instead of submit: > org.apache.geode.internal.util.concurrent.CustomEntryConcurrentHashMap.clear() > org.apache.geode.internal.cache.DiskStoreImpl.executeDiskStoreTask(Runnable) > org.apache.geode.internal.cache.lru.HeapEvictor.onEvent(MemoryEvent) > org.apache.geode.distributed.internal.InternalLocator.restartWithDS(InternalDistributedSystem, > GemFireCacheImpl) > org.apache.geode.distributed.internal.FunctionExecutionPooledExecutor.FunctionExecutionPooledExecutor(BlockingQueue, > int, PoolStatHelper, ThreadFactory, int, boolean) > org.apache.geode.internal.cache.PRHARedundancyProvider.scheduleCreateMissingBuckets() > org.apache.geode.distributed.internal.InternalLocator.startSharedConfigurationService(GemFireCacheImpl) > org.apache.geode.cache.client.internal.SingleHopClientExecutor.submitTask(Runnable) > org.apache.geode.management.internal.FederatingManager.submitTask(Callable) -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (GEODE-2109) calling submit on ExecutionService can cause exceptions to be lost
[
https://issues.apache.org/jira/browse/GEODE-2109?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15706609#comment-15706609
]
ASF GitHub Bot commented on GEODE-2109:
---
Github user upthewaterspout commented on a diff in the pull request:
https://github.com/apache/incubator-geode/pull/296#discussion_r90118721
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/FederatingManager.java
---
@@ -386,7 +398,7 @@ public void startManagingActivity() throws Exception {
*
*/
- private class GIITask implements Callable {
+ private class GIITask implements Callable {
--- End diff --
Why did you remove from this class declaration?
> calling submit on ExecutionService can cause exceptions to be lost
> --
>
> Key: GEODE-2109
> URL: https://issues.apache.org/jira/browse/GEODE-2109
> Project: Geode
> Issue Type: Bug
> Components: regions
>Reporter: Darrel Schneider
>
> Geode has a number of places that call submit on ExecutionService. The submit
> method returns a Future object. If the caller makes sure it calls "get" on
> the Future then all is well. But in many places geode is not calling get. In
> that case if the Runnable that was submitted throws an exception it gets
> stored in the get and never logged. This can make it very hard to diagnose
> problems.
> If the caller does not want to call get on the returned Future then it should
> instead call the "execute" method. In that case the exception will be
> unhandled and the unhandled exception handler code we have on the
> LoggingThreadGroup class will cause the exception to be logged.
> Here are the places that should be changed to use execute instead of submit:
> org.apache.geode.internal.util.concurrent.CustomEntryConcurrentHashMap.clear()
> org.apache.geode.internal.cache.DiskStoreImpl.executeDiskStoreTask(Runnable)
> org.apache.geode.internal.cache.lru.HeapEvictor.onEvent(MemoryEvent)
> org.apache.geode.distributed.internal.InternalLocator.restartWithDS(InternalDistributedSystem,
> GemFireCacheImpl)
> org.apache.geode.distributed.internal.FunctionExecutionPooledExecutor.FunctionExecutionPooledExecutor(BlockingQueue,
> int, PoolStatHelper, ThreadFactory, int, boolean)
> org.apache.geode.internal.cache.PRHARedundancyProvider.scheduleCreateMissingBuckets()
> org.apache.geode.distributed.internal.InternalLocator.startSharedConfigurationService(GemFireCacheImpl)
> org.apache.geode.cache.client.internal.SingleHopClientExecutor.submitTask(Runnable)
> org.apache.geode.management.internal.FederatingManager.submitTask(Callable)
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (GEODE-2109) calling submit on ExecutionService can cause exceptions to be lost
[ https://issues.apache.org/jira/browse/GEODE-2109?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15697222#comment-15697222 ] ASF GitHub Bot commented on GEODE-2109: --- GitHub user deepakddixit opened a pull request: https://github.com/apache/incubator-geode/pull/296 GEODE-2109 : Calling submit on ExecutionService can cause exceptions to be lost Replaced submit call with execute. Refactored GIITask calls from DiskStoreImpl. Async task like Creating KRF's through disk store are replaced with execute. The other call is kept with submit as we later check for whether delayed write is completed or not. Added RejectedExecutionException to IgnoredException list as from these tests it may get occasionally thrown and as we moved to execute method of ExecutorService they will be logged. All precheckin tests are passing except one org.apache.geode.management.internal.cli.commands.IndexCommandsDUnitTest.testCreateDestroyUpdatesSharedConfig which is marked as Flaky. You can merge this pull request into a Git repository by running: $ git pull https://github.com/deepakddixit/incubator-geode bugfix/GEODE-2109 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-geode/pull/296.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 #296 commit 8c4aeaae23d242b9a4edfe328bd4046e5626914d Author: Deepak Dixit Date: 2016-11-23T16:03:14Z GEODE-2109: Replacing ExecutorService.submit calls with ExecutorService.execute call. This will help in logging exceptions those come in the thread. commit db213bb1d0982db12ffc5e8d2649cec8f69147c6 Author: Deepak Dixit Date: 2016-11-24T06:27:58Z GEODE-2109: Replacing ExecutorService.submit calls with ExecutorService.execute call for DiskStoreTask. Refactored DiskStore task for delayed writes, we cannot replace expensive writes tasks submit call with execute as later we check if write call is completed or not. Replaced submit call by execute in case of DiskStore tasks like compactions, creating KRF's. commit 478a714979ace2f187ebf336c0d5121a0a5c8940 Author: Deepak Dixit Date: 2016-11-24T12:21:54Z GEODE-2109: Replacing ExecutorService.submit calls with ExecutorService.execute call for DiskStoreTask. Replaced submit call for GIITask and RemoveMember tasks. GIITask is used when adding member or starting managing activity when node becomes managing node. While adding member we can make the ExecutorService.execute call. Not changed call for GIITask when invoked from managing activity as possible exception is handled. commit b501654a3718f3e15cc45b9bd757bfb558f664a1 Author: Deepak Dixit Date: 2016-11-24T16:27:38Z GEODE-2109: Replacing ExecutorService.submit calls with ExecutorService.execute call for DiskStoreTask. Ignoring rejectedException as this can be thrown in case pool is shutting down. commit 9e56037ac21768adde9c35c97bc8e0cb463042b7 Author: Deepak Dixit Date: 2016-11-24T17:17:53Z GEODE-2109: Replacing ExecutorService.submit calls with ExecutorService.execute call for DiskStoreTask. Changing way creating GIITask when node becomes managing node. commit 3b8273b7746f5b4af329c92c92fac07e6571cf05 Author: Deepak Dixit Date: 2016-11-25T05:15:26Z GEODE-2109: Replacing ExecutorService.submit calls with ExecutorService.execute call for DiskStoreTask. Adding ignoredException for MemoryThresholdsDUnitTest. commit 5cf0aac229c595b91436c4d327d43444120698b5 Author: Deepak Dixit Date: 2016-11-25T08:05:39Z GEODE-2109: Replacing ExecutorService.submit calls with ExecutorService.execute call for DiskStoreTask. Adding ignoredException for EvictionStatsDUnitTest. commit ed24011d0b1456b0247bd0254af252cae351eb07 Author: Deepak Dixit Date: 2016-11-26T04:02:26Z GEODE-2109: Replacing ExecutorService.submit calls with ExecutorService.execute call for DiskStoreTask. Applying spotless checks. > calling submit on ExecutionService can cause exceptions to be lost > -- > > Key: GEODE-2109 > URL: https://issues.apache.org/jira/browse/GEODE-2109 > Project: Geode > Issue Type: Bug > Components: regions >Reporter: Darrel Schneider > > Geode has a number of places that call submit on ExecutionService. The submit > method returns a Future object. If the caller makes sure it calls "get" on > the Future then all is well. But in many places geode is not calling get. In > that case if the Runnable that was submitted throws an exception it gets > stored in the get and never logged. This can make it very hard to diagnose > problems. > If the caller does not want to call g
