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 pendingTasks accounting if TimedRunnable disposed before scheduling #3780

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

kkondratov
Copy link
Contributor

Addresses the issue with TimedScheduler pending task leaks as described in #3697

The chosen approach splits existing TimedRunnable into two implementations and moves code from the TimedScheduler into the TimedRunnable to reduce duplication.
Two implementations WorkerBacked/SchedulerBackedTimedRunnable are present to differentiate between scheduler or a worker that schedules a task (since there is no shared interface between those).
Differentiating from the initial proposed in #3697 by @nathankooij there is no extra objects being created and the scheduling is delegated to TimedRunnable.

Fixes #3697

@kkondratov kkondratov requested a review from a team as a code owner April 4, 2024 13:10
Copy link
Member

@chemicL chemicL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Thanks :) Please rebase to target the 3.5.x branch and we can merge.

@kkondratov kkondratov changed the base branch from main to 3.5.x April 10, 2024 13:50
@kkondratov
Copy link
Contributor Author

@chemicL I think I've rebased correctly and targeting 3.5.x branch now as requested.

@chemicL chemicL added this to the 3.5.17 milestone Apr 11, 2024
@chemicL chemicL merged commit 43bf82d into reactor:3.5.x Apr 11, 2024
7 checks passed
chemicL added a commit that referenced this pull request Apr 11, 2024
chemicL added a commit that referenced this pull request Apr 11, 2024
@chemicL chemicL changed the title Fixes issue #3697 disposing TimedRunnable if disposed before scheduling Fix pendingTasks accounting if TimedRunnable disposed before scheduling Apr 11, 2024
@chemicL
Copy link
Member

chemicL commented Apr 11, 2024

Thank you for the contribution @kkondratov! Great job and please consider contributing in the future :) 🚀

chemicL added a commit that referenced this pull request Apr 11, 2024
@chemicL
Copy link
Member

chemicL commented Apr 11, 2024

Please note the new tests failed when the target branch CI job was run: https://github.com/reactor/reactor-core/actions/runs/8643683792/job/23697225902

I corrected the flakiness of the tests in d11ea71 – please have a look. The tests assumed that active and pending are both equal to 1 when one task is able to run and the other should be waiting. However a situation can happen in which the Runnable is not yet being executed and has not yet decremented the pending counter.

@chemicL
Copy link
Member

chemicL commented Apr 11, 2024

There are other issues of such nature in that test class, e.g. ignoring the result of CountDownLatch#await - 10 seconds can pass and the call returns false, while the test should immediately fail. Feel free to open a PR if you're inclined to improve it :)

chemicL added a commit that referenced this pull request Apr 11, 2024
chemicL added a commit that referenced this pull request Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TimedScheduler leaks cancelled pending task samples
2 participants