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

Rewrite setTimeout / setImmediate behavior #70

Merged
merged 5 commits into from
Oct 19, 2021

Conversation

joshkel
Copy link
Contributor

@joshkel joshkel commented Oct 19, 2021

This revises fake-indexeddb's scheduling to work in Jest 27 + jsdom while still remanining performant. (See #69, #67, and related issues.) To review / summarize my current understanding:

  • Node.js supports setImmediate (schedule a task for the next event loop) and setTimeout (schedule a task for a subsequent event loop, with a minimum duration of 1ms, with 15ms resolution on Windows). Web browsers support setTimeout (with a minimum duration of 0ms). jsdom supports setTimeout (but uses Node.js's implementation, so it's limited in its duration and resolution). jsdom with Jest 27 recently removed support for setImmediate.
  • Within the event loop, Node.js runs the "next tick" queue (process.nextTick), then it runs the microtask queue (promises and queueMicrotask), then it resumes the event loop (and runs the next "macrotask"). (See here and here.)
  • For the purposes of IndexedDB, the important bit is that a transaction's active flag is set to false (i.e., autocommit behavior is invoked) when the event loop resumes. In other words, fake-indeeddb cannot mark transactions as finished within process.nextTick or microtasks. (See here and here.)

I initially thought that I could improve performance by only using setTimeout to finish transactions and (to avoid setTimeout's slowness) using the microtask queue for everything else. This mostly worked, but I ran into a test failure because microtasks allowed fake-indexeddb database deletion operations to be interleaved with test code, producing out-of-order results. I fixed that by moving database deletion to setTimeout, which broke another test invoking database opening, and moving database opening to setTimeout broke another test. (See intermediate commits in this PR.)

So that approach didn't seem like it would pan out, so I gave up and broke jsdom's sandbox to access setImmediate, based on scala-js/scala-js-macrotask-executor#17 and jsdom/jsdom#2729. (It turns out that fake-indexeddb is not the first project to encounter this issue.) This is a dirty hack, but it's documented as a possibility, and it was really easy, and another project is using it.

Some tests are failing or timing out.
The only failing test was delete-request-queue.js, but fixing that broke idbfactory-open-error-properties.js, and fixing idbfactory-open-error-properties.js broke idbindex-query-exception-order.js.  This strategy may not work.
Fixing one test seemed to break another; I'm not certain that IndexedDB's expected timing and order of operations can be made to work within the microtask queue.

Instead, break jsdom's sandbox so that we can use Node.js's setImmediate after all:

- jsdom/jsdom#2729
- scala-js/scala-js-macrotask-executor#17
@dumbmatter
Copy link
Owner

so I gave up and broke jsdom's sandbox

lol :)

This is great. Thank you so much! It does indeed seem to fix the problem, so I will release a new version shortly.

@dumbmatter dumbmatter merged commit cd1fa50 into dumbmatter:master Oct 19, 2021
@joshkel joshkel deleted the timing-changes branch October 19, 2021 22:05
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