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

Fix race condition in task scheduling #10892

Merged
merged 1 commit into from
Jun 7, 2024
Merged

Conversation

yawkat
Copy link
Member

@yawkat yawkat commented Jun 7, 2024

Under some circumstances, ExecutableMethodProcessor.process may be called during or even after the StartupEvent is fired. In that case, scheduled methods may be ignored or there may be a ConcurrentModificationException.

This patch moves the schedule logic into its own method, called by a class ScheduleTaskRunnable that guards the schedule logic using an AtomicBoolean so that it is run only once. Inside process(), the task is created, registered in a ConcurrentHashMap and, if the StartupEvent has already been fired, invoked. Inside the StartupEvent listener, all previously registered tasks are invoked.

  • If process is called before the started flag is set, the task will definitely be run by the StartupEvent listener.
  • If process is called after the started flag is set, the task will definitely be called inside process, and potentially also inside the StartupEvent listener if that is still running.

This ensures that the ScheduleTaskRunnable is invoked at least once.

Under some circumstances, ExecutableMethodProcessor.process may be called during or even after the StartupEvent is fired. In that case, scheduled methods may be ignored or there may be a ConcurrentModificationException.

This patch moves the schedule logic into its own method, called by a class ScheduleTaskRunnable that guards the schedule logic using an AtomicBoolean so that it is run only once. Inside process(), the task is created, registered in a ConcurrentHashMap and, if the StartupEvent has already been fired, invoked. Inside the StartupEvent listener, all previously registered tasks are invoked.

- If process is called before the started flag is set, the task will definitely be run by the StartupEvent listener.
- If process is called after the started flag is set, the task will definitely be called inside process, and potentially also inside the StartupEvent listener if that is still running.

This ensures that the ScheduleTaskRunnable is invoked at least once.
@yawkat
Copy link
Member Author

yawkat commented Jun 7, 2024

The diff is unfortunately pretty big, but you can ignore scheduleTask. It just contains the code that was previously in scheduleTasks.

@yawkat yawkat added the type: bug Something isn't working label Jun 7, 2024
@yawkat yawkat added this to the 4.5.3 milestone Jun 7, 2024
Copy link

sonarcloud bot commented Jun 7, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
1 New Critical Issues (required ≤ 0)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@graemerocher graemerocher merged commit a0ff610 into 4.5.x Jun 7, 2024
16 of 17 checks passed
@graemerocher graemerocher deleted the concurrent-schedule branch June 7, 2024 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants