You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/02/23 18:49:19 UTC
[GitHub] [accumulo] dlmarion opened a new issue #2520: Exceptions and Errors ignored due to unchecked Futures
dlmarion opened a new issue #2520:
URL: https://github.com/apache/accumulo/issues/2520
**Describe the bug**
@keith-turner reported an issue where he had the wrong `accumulo-core` jar file deployed to a cluster and the TabletServer continued to run. When he changed the code to catch `Throwable` instead of catch `Exception`, this stacktrace was captured:
```
java.lang.NoSuchFieldError: TSERV_WAL_BUFFER
at org.apache.accumulo.tserver.log.DfsLogger.open(DfsLogger.java:425) ~[accumulo-tserver-2.1.0-SNAPSHOT.jar:2.1.0-SNAPSHOT]
at org.apache.accumulo.tserver.log.TabletServerLogger$2.run(TabletServerLogger.java:277) ~[accumulo-tserver-2.1.0-SNAPSHOT.jar:2.1.0-SNAPSHOT]
at org.apache.accumulo.core.trace.TraceWrappedRunnable.run(TraceWrappedRunnable.java:52) ~[accumulo-core-2.1.0-SNAPSHOT.jar:2.1.0-SNAPSHOT]
at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515) ~[?:?]
at java.util.concurrent.FutureTask.run(FutureTask.java:264) ~[?:?]
at org.apache.accumulo.core.trace.TraceWrappedRunnable.run(TraceWrappedRunnable.java:52) ~[accumulo-core-2.1.0-SNAPSHOT.jar:2.1.0-SNAPSHOT]
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) ~[?:?]
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) ~[?:?]
at org.apache.accumulo.core.trace.TraceWrappedRunnable.run(TraceWrappedRunnable.java:52) ~[accumulo-core-2.1.0-SNAPSHOT.jar:2.1.0-SNAPSHOT]
at java.lang.Thread.run(Thread.java:829) [?:?]
```
It turns out that the issue here is that `ExecutorService.submit` was being called and the `Future` result object was not being checked. When he changed the [code](https://github.com/apache/accumulo/blob/main/server/tserver/src/main/java/org/apache/accumulo/tserver/log/TabletServerLogger.java#L266) to use `ExecutorService.execute` instead, then the `AccumuloUncaughtExceptionHandler` was invoked, the Error printed in the log, and the VM halted. WHen using `submit`, the TabletServer remained up, but in a Zombie non-functional state.
It appears that Futures are capturing Exceptions and Errors, so the UncaughtExceptionHandler is not being invoked. It would be nice if SpotBugs had a rule for this. The closest I found is https://github.com/spotbugs/spotbugs/issues/950, but it's still open.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on issue #2520: Exceptions and Errors ignored due to unchecked Futures
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #2520:
URL: https://github.com/apache/accumulo/issues/2520#issuecomment-1049463464
We probably shouldn't be adding SuppressWarnings for errorprone-specific warnings. I've found it to be quite annoying in Eclipse (and perhaps in other IDEs) when warnings that it doesn't recognize are suppressed. In such cases, it actually generates new warnings. But, the only option to hide those warnings about "unnecessary suppressions" is to turn that off entirely... and then it doesn't catch legitimate typos for standard warnings suppressions. I hit this hard when I found that Eclipse doesn't recognize a standard warning "overloads": https://bugs.eclipse.org/bugs/show_bug.cgi?id=559000
I would prefer ignoring warnings categorically that we don't care about in the POM, and have it just fail the build (when that profile is active) when it catches something we do care about. I would prefer to avoid suppressions.
As for this bug with regard to Futures, does this only affect 2.1.0-SNAPSHOT? Or does it affect anything else?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on issue #2520: Exceptions and Errors ignored due to unchecked Futures
Posted by GitBox <gi...@apache.org>.
dlmarion commented on issue #2520:
URL: https://github.com/apache/accumulo/issues/2520#issuecomment-1049170241
It looks like ErrorProne has something for this. https://errorprone.info/bugpattern/FutureReturnValueIgnored @jmark99 - didn't you just add something with ErrorProne?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 commented on issue #2520: Exceptions and Errors ignored due to unchecked Futures
Posted by GitBox <gi...@apache.org>.
jmark99 commented on issue #2520:
URL: https://github.com/apache/accumulo/issues/2520#issuecomment-1049246444
Removing the DisableAllWarnings is fine, but ErrorProne is fairly aggressive so there will be a lot of SuppressWarning annotations to add. Of course once all warnings are parsed you can always update the pom to ignore warnings we do not care about.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion closed issue #2520: Exceptions and Errors ignored due to unchecked Futures
Posted by GitBox <gi...@apache.org>.
dlmarion closed issue #2520:
URL: https://github.com/apache/accumulo/issues/2520
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on issue #2520: Exceptions and Errors ignored due to unchecked Futures
Posted by GitBox <gi...@apache.org>.
keith-turner commented on issue #2520:
URL: https://github.com/apache/accumulo/issues/2520#issuecomment-1049925009
> As for this bug with regard to Futures, does this only affect 2.1.0-SNAPSHOT? Or does it affect anything else?
The place that started this investigation may not be a problem in 1.10.x because it uses the old LoggingRunnable class which catches and logs throwables. However, there could be other places in 1.10 that submit/schedule a future and do not use LoggingRunnable. It would be good to investigate this 1.10 errorprone to find where futures are ignored and see if those instances are problematic.
https://github.com/apache/accumulo/blob/e62816675c51b48ae438766663746b3ce5191251/server/tserver/src/main/java/org/apache/accumulo/tserver/log/TabletServerLogger.java#L273
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on issue #2520: Exceptions and Errors ignored due to unchecked Futures
Posted by GitBox <gi...@apache.org>.
keith-turner commented on issue #2520:
URL: https://github.com/apache/accumulo/issues/2520#issuecomment-1049110397
I was running into this problem where I knew my exception was being mysteriously eaten and was not sure why. I reached out to @dlmarion on slack and he tracked the problem down in short order. The crazy situation that created this problem was :
* I added a new property to Accumulo locally for an experiment I was running
* I used this property in the tserver code
* I had an existing cluster where I was running experiments and I only copied up the tserver jar (not the core jar w/ the new enum definition).
* The tserver code threw the java.lang.NoSuchFieldError because I did not copy the core jar and that exception was silently dropped.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on issue #2520: Exceptions and Errors ignored due to unchecked Futures
Posted by GitBox <gi...@apache.org>.
dlmarion commented on issue #2520:
URL: https://github.com/apache/accumulo/issues/2520#issuecomment-1049179981
I added the following to the `errorprone` profile in the parent pom
```
-Xep:FutureReturnValueIgnored:ERROR \
```
It found 16 errors in the tserver module when I ran `mvn -pl server/tserver -Perrorprone clean package -DskipTests`
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on issue #2520: Exceptions and Errors ignored due to unchecked Futures
Posted by GitBox <gi...@apache.org>.
dlmarion commented on issue #2520:
URL: https://github.com/apache/accumulo/issues/2520#issuecomment-1049214968
I think we should remove `-XepDisableAllWarnings` from the errorprone profile configuration so that we can evaluate potential issues. We can use `@SuppressWarnings` to deal with the warnings that we are ok with.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner edited a comment on issue #2520: Exceptions and Errors ignored due to unchecked Futures
Posted by GitBox <gi...@apache.org>.
keith-turner edited a comment on issue #2520:
URL: https://github.com/apache/accumulo/issues/2520#issuecomment-1049110397
I was running into this problem where I knew my exception was being mysteriously eaten and was not sure why. I reached out to @dlmarion on slack and he tracked the problem down in short order. The crazy situation that created this problem was :
* I added a new property to Accumulo locally for an experiment I was running
* I used this new property in the tserver code
* I had an existing cluster where I was running experiments and I only copied up the tserver jar (not the core jar w/ the new enum definition).
* The tserver code threw the java.lang.NoSuchFieldError (because I did not copy the core jar) and that exception was silently dropped.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on issue #2520: Exceptions and Errors ignored due to unchecked Futures
Posted by GitBox <gi...@apache.org>.
dlmarion commented on issue #2520:
URL: https://github.com/apache/accumulo/issues/2520#issuecomment-1049953319
I have "fixed" a couple of these issues. I'm thinking about pushing my branch to the accumulo repo vs my fork. That would allow others to check it out and see some of the errors. The approach I have been taking is:
1. If `ThreadPoolExecutor.submit` is being called and the resulting `Future` is not being captured, then change the method call to `ThreadPoolExecutor.execute`.
2. If `ScheduledThreadPoolExecutor` is being used, then:
a. Do nothing if the failure of the scheduled task would not cause some type of error. For example if the task is logging something or updating metrics
b. Check `ScheduledFuture.isDone` in some other location to see if the background task has completed (failed).
I used the following test to check that `isDone` will return true when the task throws an error. Note that `isCancelled` is `false` when an exception is thrown, `get` will not return, and `get(int, TimeUnit)` will throw a TimeoutException. `isDone` seemed the only way to check that an error is thrown.
```
@Test
public void test() throws Exception {
final AtomicInteger counter = new AtomicInteger(0);
ScheduledThreadPoolExecutor pool = ThreadPools.createScheduledExecutorService(1, "test", false);
ScheduledFuture<?> f = pool.scheduleAtFixedRate(() -> {
if (counter.incrementAndGet() == 3) {
throw new RuntimeException();
}
}, 1, 1, TimeUnit.SECONDS);
Assert.assertFalse(f.isCancelled());
Assert.assertFalse(f.isDone());
Assert.assertThrows(TimeoutException.class, () -> f.get(900, TimeUnit.MILLISECONDS));
Assert.assertFalse(f.isCancelled());
Assert.assertFalse(f.isDone());
Assert.assertThrows(TimeoutException.class, () -> f.get(900, TimeUnit.MILLISECONDS));
Assert.assertFalse(f.isCancelled());
Assert.assertFalse(f.isDone());
Assert.assertThrows(TimeoutException.class, () -> f.get(900, TimeUnit.MILLISECONDS));
Assert.assertFalse(f.isCancelled());
Assert.assertFalse(f.isDone());
Assert.assertThrows(ExecutionException.class, () -> f.get(900, TimeUnit.MILLISECONDS));
Assert.assertFalse(f.isCancelled());
Assert.assertTrue(f.isDone());
}
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on issue #2520: Exceptions and Errors ignored due to unchecked Futures
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #2520:
URL: https://github.com/apache/accumulo/issues/2520#issuecomment-1049980022
> I'm thinking about pushing my branch to the accumulo repo vs my fork. That would allow others to check it out and see some of the errors. The approach I have been taking is:
A Draft PR to the repo from your fork would be better than pushing the working branch to the accumulo repo. Commits pushed to working branches in the repo still generate commits@ notifications, which some people follow, and it's frustrating because these are difficult to distinguish from commits to our actively maintained branches. A PR (Draft or otherwise) also provides a much better UI for seeing what files have changed and for doing code-reviews/comments on those changes.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on issue #2520: Exceptions and Errors ignored due to unchecked Futures
Posted by GitBox <gi...@apache.org>.
dlmarion commented on issue #2520:
URL: https://github.com/apache/accumulo/issues/2520#issuecomment-1050854516
I'm seeing an issue when dealing with the `ScheduledFutures` from the `ScheduledThreadPoolExecutors`. Specifically, if the future is never checked, then we won't know that an Exception or an Error occurred, and that the task is not running. It may be the case that the probability is low of an Exception or Error occurring, but if it did then we would never know. I'm wondering if instead of scheduling the task to run at periodic intervals we modify the Runnable into a loop with a sleep interval. By submitting the Runnable via `execute` instead of `submit`, a Future is not returned and the `AccumuloUncaughtExceptionHandler` will be invoked. Here are some references from server-base in addition to some that I have already fixed in this PR. I haven't progressed to the server processes yet, but it would be good to know if this approach is acceptable. Currently I'm assigning the resulting Future to a variable and checking it if possible, otherwise putting a `@SuppressWarnings("unused")`
on it.
https://github.com/apache/accumulo/blob/main/server/base/src/main/java/org/apache/accumulo/server/manager/LiveTServerSet.java#L257
https://github.com/apache/accumulo/blob/main/server/base/src/main/java/org/apache/accumulo/server/zookeeper/DistributedWorkQueue.java#L231
https://github.com/apache/accumulo/blob/main/server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java#L317
https://github.com/apache/accumulo/blob/main/server/base/src/main/java/org/apache/accumulo/server/fs/FileManager.java#L166
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on issue #2520: Exceptions and Errors ignored due to unchecked Futures
Posted by GitBox <gi...@apache.org>.
dlmarion commented on issue #2520:
URL: https://github.com/apache/accumulo/issues/2520#issuecomment-1050881880
Using the loop/sleep approach described above may not be desirable as the total number of threads in the JVM will go up and threads will not be re-used. Another approach at resolving this would be to register the ScheduledFutures somewhere and have 1 thread periodically check them all to make sure they are still running. I think I'll go down this path for now.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on issue #2520: Exceptions and Errors ignored due to unchecked Futures
Posted by GitBox <gi...@apache.org>.
dlmarion commented on issue #2520:
URL: https://github.com/apache/accumulo/issues/2520#issuecomment-1049802022
> As for this bug with regard to Futures, does this only affect 2.1.0-SNAPSHOT? Or does it affect anything else?
It's hard to tell how pervasive the issue is because setting `-Xep:FutureReturnValueIgnored:ERROR` fails the build after the first module where the issue is found. My guess is that there are a fair amount of occurrences and some of them need to be addressed. In the case where we are not checking the Future result, we could easily just call another method on the ExecutorService. The ScheduledExecutorService is a little different because all of the schedule methods return ScheduledFuture objects. We would need to backport the errorprone profile to the 1.10 branch and run it there to know for sure.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org