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

Use setTimeout for async work #922

Merged
merged 6 commits into from
Oct 2, 2024
Merged

Conversation

nikku
Copy link
Member

@nikku nikku commented Jul 4, 2024

Proposed Changes

Requests to requestAnimationFrame may be dropped, depending on how the browser feels. This is what makes tests under Ubuntu/Firefox fail, but it may also cause issues in other high stress scenarios.

If we want to reliably ensure work gets done, we have to setTimeout(work) things. This is what I propose in 8f33e8b.

Closes #921

Checklist

To ensure you provided everything we need to look at your PR:

  • Brief textual description of the changes present
  • Visual demo attached
  • Steps to try out present, i.e. using the @bpmn-io/sr tool
  • Related issue linked via Closes {LINK_TO_ISSUE} or Related to {LINK_TO_ISSUE}

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Jul 4, 2024
@nikku nikku added the ready Ready to be worked on label Aug 21, 2024 — with bpmn-io-tasks
@nikku nikku removed the in progress Currently worked on label Aug 21, 2024
Using a local profile directory, this works on modern Linux, too:

karma-runner/karma-firefox-launcher#183 (comment)
@nikku
Copy link
Member Author

nikku commented Oct 1, 2024

This can be reproduced somewhat locally using the Firefox headless runner, but only when running the whole test suite. Executing only the scheduler specs passes.

Current assumption is that we don't properly cleanup the scheduler, or that Firefox swallows the requestAnimationFrame calls (likely).

nikku added 5 commits October 1, 2024 22:28
Requests to requestAnimationFrame may be dropped, depending on
how the browser feels. If we want to reliably ensure work gets done,
we have to setTimeout(work) things.

Closes #921
@nikku nikku force-pushed the 921-firefox-scheduler-test-fix branch from 923b948 to fadba41 Compare October 1, 2024 20:29
@nikku
Copy link
Member Author

nikku commented Oct 1, 2024

As suspected this is about Firefox dropping frames under high load (aka huge page) setups.

In order to reliably do stuff we have to await or setTimeout (can be canceled) it, cf. 8f33e8b.

@nikku nikku marked this pull request as ready for review October 1, 2024 20:30
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed ready Ready to be worked on labels Oct 1, 2024
@nikku nikku changed the title 921 firefox scheduler test fix Use setTimeout for async work Oct 1, 2024
@nikku nikku requested review from marstamm, a team and barmac and removed request for a team October 1, 2024 20:35
@nikku
Copy link
Member Author

nikku commented Oct 1, 2024

Executing test on Firefox takes whooping 12 minutes:

capture J7ZIYn_optimized

@barmac
Copy link
Member

barmac commented Oct 2, 2024

Hmm maybe we should not rush it. This causes some tests to fail on my machine (MacOS, Chrome Headless 129).

@barmac
Copy link
Member

barmac commented Oct 2, 2024

I'll check on fresh install.

@barmac
Copy link
Member

barmac commented Oct 2, 2024

OK so I guess I am now suffering from the same problem as Firefox before the changes. On main, I can npm run all all green, but with this PR, the tests start to timeout :/

@barmac
Copy link
Member

barmac commented Oct 2, 2024

And the tests are very old, rather unrelated to scheduler.

@nikku nikku merged commit 2369342 into develop Oct 2, 2024
12 checks passed
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Oct 2, 2024
@nikku nikku deleted the 921-firefox-scheduler-test-fix branch October 2, 2024 11:54
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.

Fix Scheduler tests consistently failing on CI (Linux, Firefox)
2 participants