-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Catch exceptions in scheduled tasks to prevent unintended cancellation #12853
Merged
lhotari
merged 3 commits into
apache:master
from
lhotari:lh-prevent-scheduled-task-cancellation
Nov 19, 2021
Merged
Catch exceptions in scheduled tasks to prevent unintended cancellation #12853
lhotari
merged 3 commits into
apache:master
from
lhotari:lh-prevent-scheduled-task-cancellation
Nov 19, 2021
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- ScheduledExecutorService#scheduleAtFixedRate won't schedule the next execution if running the task throws an exception. This can lead to unintended cancellation of the scheduled task in failure scenarios.
eolivelli
requested changes
Nov 17, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me
I left some feedback
pulsar-common/src/main/java/org/apache/pulsar/common/util/Runnables.java
Outdated
Show resolved
Hide resolved
pulsar-common/src/main/java/org/apache/pulsar/common/util/Runnables.java
Show resolved
Hide resolved
pulsar-common/src/main/java/org/apache/pulsar/common/util/Runnables.java
Outdated
Show resolved
Hide resolved
eolivelli
approved these changes
Nov 17, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
merlimat
approved these changes
Nov 17, 2021
dlg99
pushed a commit
to dlg99/pulsar
that referenced
this pull request
Nov 23, 2021
apache#12853) * Catch exceptions in scheduled tasks to prevent unintended cancellation - ScheduledExecutorService#scheduleAtFixedRate won't schedule the next execution if running the task throws an exception. This can lead to unintended cancellation of the scheduled task in failure scenarios. * Address review feedback: use private constructor * Address review feedback: Use private inner class instead of Lambda
eolivelli
pushed a commit
to eolivelli/pulsar
that referenced
this pull request
Nov 29, 2021
apache#12853) * Catch exceptions in scheduled tasks to prevent unintended cancellation - ScheduledExecutorService#scheduleAtFixedRate won't schedule the next execution if running the task throws an exception. This can lead to unintended cancellation of the scheduled task in failure scenarios. * Address review feedback: use private constructor * Address review feedback: Use private inner class instead of Lambda
lhotari
added a commit
that referenced
this pull request
Dec 9, 2021
#12853) * Catch exceptions in scheduled tasks to prevent unintended cancellation - ScheduledExecutorService#scheduleAtFixedRate won't schedule the next execution if running the task throws an exception. This can lead to unintended cancellation of the scheduled task in failure scenarios. * Address review feedback: use private constructor * Address review feedback: Use private inner class instead of Lambda (cherry picked from commit afdfe19)
lhotari
added a commit
that referenced
this pull request
Dec 9, 2021
#12853) * Catch exceptions in scheduled tasks to prevent unintended cancellation - ScheduledExecutorService#scheduleAtFixedRate won't schedule the next execution if running the task throws an exception. This can lead to unintended cancellation of the scheduled task in failure scenarios. * Address review feedback: use private constructor * Address review feedback: Use private inner class instead of Lambda (cherry picked from commit afdfe19)
lhotari
added a commit
to datastax/pulsar
that referenced
this pull request
Dec 10, 2021
apache#12853) * Catch exceptions in scheduled tasks to prevent unintended cancellation - ScheduledExecutorService#scheduleAtFixedRate won't schedule the next execution if running the task throws an exception. This can lead to unintended cancellation of the scheduled task in failure scenarios. * Address review feedback: use private constructor * Address review feedback: Use private inner class instead of Lambda (cherry picked from commit afdfe19) (cherry picked from commit 7bd69f9)
fxbing
pushed a commit
to fxbing/pulsar
that referenced
this pull request
Dec 19, 2021
apache#12853) * Catch exceptions in scheduled tasks to prevent unintended cancellation - ScheduledExecutorService#scheduleAtFixedRate won't schedule the next execution if running the task throws an exception. This can lead to unintended cancellation of the scheduled task in failure scenarios. * Address review feedback: use private constructor * Address review feedback: Use private inner class instead of Lambda
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
cherry-picked/branch-2.8
Archived: 2.8 is end of life
cherry-picked/branch-2.9
Archived: 2.9 is end of life
doc-not-needed
Your PR changes do not impact docs
release/2.8.2
release/2.9.1
type/cleanup
Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use
type/enhancement
The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Prevent the intended cancellation of scheduled tasks in Pulsar code base.
ScheduledExecutorService#scheduleAtFixedRate (and scheduleWithFixedDelay) won't schedule the next execution if running the task throws an exception. This is explained in the javadoc. This can lead to unintended cancellation of the scheduled task in failure scenarios.
There are some failures scenarios where it is hard to determine what causes Pulsar broker or client to get into a state where the service is partially working. One such source of issues could be such that a scheduled task silently gets cancelled. This PR will ensure that this won't happen when scheduled tasks are using the provided solution (Runnables.catchingAndLoggingThrowables).
Additional context
This is a cross-cutting change that is about the correct usage of ScheduleExecutorService API. The problem and solution is explained in this StackOverflow Answer: https://stackoverflow.com/a/24902026.
Bookkeeper client includes a solution called SafeRunnable (in 2 locations: in https://github.com/apache/bookkeeper/blob/master/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/SafeRunnable.java and https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/SafeRunnable.java). The SafeRunnable.safeRun method is used extensively in many parts of Bookkeeper code. Some parts of Pulsar code base uses SafeRunnable.safeRun to catch and log exceptions.
The concept of "SafeRunnable" is confusing. Instead of copying the same solution that Bookkeeper uses, it is intentional that there is no "safe runnable" and the method is called "Runnables.catchingAndLoggingThrowables" to make it explicit that the Runnable is wrapped with handling that catches and logs throwables. There is no "magic".
Modifications
org.apache.pulsar.common.util.Runnables.catchingAndLoggingThrowables
to pulsar-common which wraps aRunnable
with try-catch block to catch and log all Throwables.