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: support non-unique handles for timers #71

Merged
merged 6 commits into from
Jun 26, 2024
Merged

Conversation

guybedford
Copy link
Contributor

This was necessary to get one of the WPT cases to pass for Fastly's implementation.

Our implementation of checking task deadlines for timer tasks uses a redundant singular invalid handle (-2) to indicate a timer handle for a non-existent subscription. If we gave every timer a unique handle we would be using up the real handle ID space.

But the cancel_task currently assumes handle uniqueness when finding the handle.

This supports non-unique handles for tasks by taking the task itself to cancel_task instead of its handle to look it up from, storing timers as a map from timer ids to task pointers.

I can confirm this is working correctly in the WPT test case for Fastly.

Copy link
Member

@tschneidereit tschneidereit left a comment

Choose a reason for hiding this comment

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

If I understand the problem correctly, all timers in Fastly's implementation have PollableHandle == -2 because of how timeouts are translated, right?

If so, that makes sense to me, yeah.

The tricky bit, and why I didn't immediately implement things this way, is that the TimerTasks in the map need to be properly rooted—see the comment in-line.

builtins/web/timers.cpp Show resolved Hide resolved
@guybedford
Copy link
Contributor Author

If I understand the problem correctly, all timers in Fastly's implementation have PollableHandle == -2 because of how timeouts are translated, right?

Yes exactly.

I've implemented a very naive singleton and trace model here as a start. Further feedback very much welcome.

Copy link
Member

@tschneidereit tschneidereit left a comment

Choose a reason for hiding this comment

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

Looks good! I left a bunch of suggestions which should be applied (and then a little bit of other tweaks as follow-ups to those, because I didn't add suggestions for all the places where s/timer_ids_/timers_/g needs to happen), but with those in, this'll be ready to go!

builtins/web/timers.cpp Outdated Show resolved Hide resolved
builtins/web/timers.cpp Outdated Show resolved Hide resolved
builtins/web/timers.cpp Outdated Show resolved Hide resolved
builtins/web/timers.cpp Outdated Show resolved Hide resolved
builtins/web/timers.cpp Outdated Show resolved Hide resolved
builtins/web/timers.cpp Outdated Show resolved Hide resolved
Copy link
Member

@tschneidereit tschneidereit left a comment

Choose a reason for hiding this comment

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

Oh huh, seems like that doesn't work with raw pointers, sorry!

The suggested edits do make it work, because tracing is implemented for js::UniquePtr.

builtins/web/timers.cpp Outdated Show resolved Hide resolved
builtins/web/timers.cpp Outdated Show resolved Hide resolved
Co-authored-by: Till Schneidereit <[email protected]>
@guybedford guybedford merged commit d7c6348 into main Jun 26, 2024
1 check passed
@guybedford guybedford deleted the timers-uniqueness-fix branch June 26, 2024 15:23
@guybedford guybedford restored the timers-uniqueness-fix branch June 26, 2024 15:23
@guybedford guybedford deleted the timers-uniqueness-fix branch June 26, 2024 15:23
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.

2 participants