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: tick timer instead of using Date.now #3494

Closed
wants to merge 1 commit into from
Closed

fix: tick timer instead of using Date.now #3494

wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Aug 23, 2024

Partially fixes some of the timeout issues we have been facing. Again needs work with tests.

Refs: #3493

This relates to...

Rationale

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

@mcollina
Copy link
Member

I'm generically ok with the change, but I would be happier with tests. Maybe one from @Uzlopak's PR?

@ronag
Copy link
Member Author

ronag commented Aug 25, 2024

I suspect that pr includes this change somewhere

@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 25, 2024

This PR is replacing the Date.now() call with saying that every time setTimeout is triggered that 499 ms are passed. This means, that if the event loop blocks for e.g. 900ms, we still "think" that just 499ms passed. So the fasttimer will be artificially delayed.

@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 25, 2024

We could change it in my PR to just increment the fastNow value instead of calling performance.now. But somehow it feels wrong.

@ronag
Copy link
Member Author

ronag commented Aug 25, 2024

The whole point is that we don't want to include event loop delay

@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 25, 2024

adapted with unit test
ad339a2

@ronag ronag closed this Aug 25, 2024
@Uzlopak Uzlopak deleted the timer-tick branch September 6, 2024 22:14
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.

3 participants