Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LoggingRunnable.run should catch and log all errors, not just Exception? #13718

Merged
merged 1 commit into from Sep 23, 2015

Conversation

@mikemccand
Copy link
Contributor

mikemccand commented Sep 22, 2015

Still digging on #13487 ... and I think it's unlikely this is the cause ...

ThreadPool.scheduleWithFixedDelay wraps the incoming command with a LoggingRunnable which runs the command, but catching, suppressing and logging any exceptions. This is important because the JDK's ScheduledThreadPoolExecutor.schedulWithFixedDelay will stop executing the task if an unhandled exception is hit.

But it only catches Exception ... I think maybe it should instead catch Throwable, so that e.g. AssertionError is also logged?

It's remotely possible this test failure is happening because some bad exception (subclassing Error) was hit and the thread is no longer checking for inactive indices ... but then I think the JDK would have sent that exception to stderr, so this is probably not it.

@nik9000

This comment has been minimized.

Copy link
Contributor

nik9000 commented Sep 22, 2015

Makes sense to me. At the root of a Runnable is the one place I've always excused catching Throwable but maybe someone with a more nuanced opinion than I can comment?

@dakrone

This comment has been minimized.

Copy link
Member

dakrone commented Sep 22, 2015

LGTM

mikemccand pushed a commit that referenced this pull request Sep 23, 2015
LoggingRunnable.run should catch and log all errors, not just Exception
@mikemccand mikemccand merged commit 4fb6386 into elastic:master Sep 23, 2015
1 check passed
1 check passed
CLA Commit author is a member of Elasticsearch
Details
@clintongormley clintongormley added v2.0.0-rc1 and removed v2.0.0 labels Oct 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.