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 clearTimeout and linux timeout #1138

Merged
merged 1 commit into from
Aug 24, 2022
Merged

Conversation

zhuzilin
Copy link
Collaborator

fix #1135

It seems that there were lots of bugs for setTimeout/setInterval. So I'll try to clarify what I'm trying to do in this PR, in case it introduces some new bugs :p

The rationale is, for each timeout event, the vm.active_tasks will be added twice:

  • one in event loop, just like any other IOTask (IOTask.createOnJSThread);
  • the other in Timer.set, which is an additional count for timer.

active_tasks will prevent bun from exit: bun will loop to wait until active_task is zero.

And correpsond to the 2 add, there will be 2 minus:

  • one in event loop, when a task is finished, event loop will minus 1 to active_task
  • the other in Timer, when a timer is triggered, and it won't happen again, active_task need to be -|= 1ed.

The reason timer events need 2 adds instead of 1 is that, when a timer event will repeat, we can just not do the second minus and keep bun alive.

There were several problem with the origin implementation.

  • when clear the timeout (clearTimer), we should do -|= 2, so that bun could exit immediately. Instead, currently it only set timer.cancelled to true.
  • no need to add to active_task when repeating, not minusing is good enough, otherwise, the setInterval cannot be cancelled.

And in this pr, I changed the code into:

  • active_tasks -|= 2 in clearTimer
  • don't active_tasks +|= 1 for repeat timer
  • don't active_tasks -|= 1 in clear, as the timer may be canceled already.

Also, for linux timeout, we should not set the count field in io_uring_prep_timeout, as it has a special function. For more information, please check: axboe/liburing#232

Thank you for your time on this PR :)

@Jarred-Sumner Jarred-Sumner merged commit e6a1209 into oven-sh:main Aug 24, 2022
@Jarred-Sumner
Copy link
Collaborator

This is great! Thank you.

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.

clearTimeout does not clear the timeout
2 participants