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

timer: fix cancel in periodic timer callback #2318

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

brain5lug
Copy link
Contributor

Fix new timers unable to set system timer callback
if there was removed timer with low new expire
time which was removed in their seastar callback

Please comment bad index calculated of timer then timer
is removed.

@avikivity
Copy link
Member

Please add a test that demonstrates the bug (fails before the fix, passes after the fix).

});

//some long 3rd party unstoppable calculation
std::this_thread::sleep_for(100ms);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this comment. Why do you need the sleep_for()? the waiting for p4's future will wait for more than 100ms anyway.
Or maybe what you're saying is that somehow the cancel() broke this sleep_for() and before this patch, the sleep_for would never finish? If this is the case, please a comment saying this, and not some statement about "long 3rd party unstoppable calculation", whatever that means.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added explicit comment that this line is for reactor delay

@nyh
Copy link
Contributor

nyh commented Jul 1, 2024

Please add a test that demonstrates the bug (fails before the fix, passes after the fix).

I see he did add a test, but never explicitly said that this test reproduces the bug - fails before the fix, and passes afterwards.
I would suggest to please:

  1. Open a github issue about the bug. Explaining exactly what the bug is and what its symptoms were (but not how to fix it).
  2. Above the test, add a comment explaing that it reproduces the issue (mentioning the issue number).
  3. Add to the pull request a "Fixes #..." line - which will cause the issue to be automatically closed when the issue is merged.

Thanks.

@brain5lug
Copy link
Contributor Author

Please add a test that demonstrates the bug (fails before the fix, passes after the fix).

The test has been already added. Probably it requires some improvements. Currently the test hangs forever (as reactor hangs) without the fix and passes the test if fix is applied. The fix is for one bug. I suspect another bug. I can prepare branch with assert which shall prove it exists.

pmelkozer added 2 commits July 1, 2024 20:38
Fix new timers unable to set system timer callback
if there was removed timer with low new expire
time which was removed in their seastar callback
Added const to to_time_point() and get_timeout() functions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants