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

[release/7.0-staging] [browser] fix job queue timespan calculation #85784

Merged

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented May 4, 2023

Backport of #85677 and #85660 to release/7.0-staging

/cc @pavelsavara

Customer Impact

"Scheduled tasks stop working after 2023.04.14 1:41:36 UTC"

Fixes #85473

Testing

Unit tests and manual testing.

Risk

Limited to wasm platform, mainly Blazor wasm.

@github-actions github-actions bot requested a review from marek-safar as a code owner May 4, 2023 17:11
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 4, 2023
@pavelsavara pavelsavara added the Servicing-consider Issue for next servicing release review label May 4, 2023
@pavelsavara pavelsavara requested a review from kg May 4, 2023 17:12
@pavelsavara pavelsavara self-assigned this May 4, 2023
@pavelsavara pavelsavara requested a review from lewing May 4, 2023 17:16
@carlossanlop
Copy link
Member

@pavelsavara - Reminder that you're free to merge your PR to the staging branch anytime, as long as:

  • It has been approved by Tactics (Servicing-approved label applied).
  • Signed-off by an area owner.
  • CI is either green, or the failures are investigated and considered unrelated.
  • OOB package authoring changes are added if needed.

If you want this fix to go into the June Release, please make sure to merge this before the code complete day (May 15th).

@pavelsavara
Copy link
Member

Thanks!
CI failures are unrelated.
I will do bit more testing before I send the email to tactics.

@pavelsavara pavelsavara modified the milestones: 8.0.0, 7.0.x May 5, 2023
@ilonatommy

This comment was marked as outdated.

@ilonatommy

This comment was marked as outdated.

@pavelsavara
Copy link
Member

pavelsavara commented May 9, 2023

It seems that time_t is 32bits in Net7 and 64bits in Net8 wasm.

We use emscripten version 3.1.12 in Net7
This seems to be the real fix emscripten-core/emscripten#17401
The fix is in 3.1.16.

@pavelsavara
Copy link
Member

pavelsavara commented May 10, 2023

We are not going to upgrade emscripten on patch. Especially when it's breaking ABI.
The change to monotonic clock we did here switches the underlying function to __clock_gettime -> _emscripten_get_now -> performance.now.

performance.now returns milliseconds since browser window start.
That is much smaller number than Date.now we had so far, which starts 1970-1-1.

The ts->tv_sec which is overflowing 31bits, will overflow in about 24000 days after the browser window was opened.
Because https://github.com/emscripten-core/emscripten/blob/d1530e7dd9c1d309fddd51b7d4b7201ec2cf3fd6/system/lib/libc/emscripten_time.c#L59-L60

I guess that's as good fix as we could get for Net7.

@carlossanlop carlossanlop added area-Threading-mono and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 10, 2023
@pavelsavara pavelsavara added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels May 12, 2023
@pavelsavara
Copy link
Member

Approved in email with Tactics.
CI timeouts are known and unrelated.
Merging

@pavelsavara pavelsavara merged commit 2b28c92 into release/7.0-staging May 12, 2023
@jkotas jkotas deleted the backport/pr-85677-to-release/7.0-staging branch May 23, 2023 14:10
@ghost ghost locked as resolved and limited conversation to collaborators Jun 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants