Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings
/avaPublic

Unref shared worker when all test workers have deregistered#3149

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

codetheweb
Copy link
Contributor

The symptom of this bug was that when shared workers are used and AVA's global timeout is triggered,ava exits with 0 instead of 1 (we make heavy use of shared workers and didn't realize until recently that our test workflow in CI is frequently marked as passing when a test times out). Similar symptom to#3098, however I think the underlying issue is quite different.

Added a test that reproduces the error and a tentative fix.

Potentially related:#3098

seveibar reacted with thumbs up emoji
@novemberborn
Copy link
Member

The changes look good, but it's unclear what the root cause of the bug is. Is AVA exiting earlier than we can set the exit code?

@codetheweb
Copy link
ContributorAuthor

codetheweb commentedDec 9, 2022
edited
Loading

I still don't completely understand the internals of AVA, so some of this might be wrong but this is what I was experiencing when running the test reproduction:

  1. The main process was stalling on
    awaitPromise.all(deregisteredSharedWorkers);
  2. The Promise array wasn't resolving because the de-registration Promise here:
    constderegistered=newPromise(resolve=>{
    never resolved.
  3. This Promise never resolved because the main process sent{type: "deregister-test-worker"} to the shared worker loader, but the shared worker loader never sent back{type: "deregistered-test-worker"} in response. I believe this was because the shared worker loader was being un-referenced too early and getting unloaded.
  4. Because the main process didn't get confirmation that the shared worker was deregistered from the test worker,registrationCount remained at1 and the de-registration Promise never resolved.

My patch fixes the reproduction and doesn't break any other tests, but I'm not completely convinced it's the correct solution. Shouldn't the main process exit with 1 if it times out for any reason (including getting stuck at

awaitPromise.all(deregisteredSharedWorkers);
?)

@novemberborn
Copy link
Member

The exit code is only set when the run completes:

process.exitCode=runStatus.suggestExitCode({matching:match.length>0});

But Node.js will exit even when you're awaiting a pending promise, if there's nothing left in the event loop that would let that promise be resolved.

Therefore we need to make sure Node.js doesn't exit on its own, otherwise we'll never set the exit code.

https://nodejs.org/api/worker_threads.html#workerunref states that the worker thread will exit when the event loop in the main thread is empty. Delaying theunref() call would therefore keep the main thread alive.

Shared workers may need to do cleanup, so we want them to (be able to) perform tasks when a test worker "goes away". That's what we're awaiting at the end of the run. (But, note that we're counting registrations of possibly different shared workersper test file.)

So the question is, why did the event loop become empty when we were still (potentially) performing cleanup? And I don't know, but it seems to be that this could be a race condition when waiting for the various events from the various worker threads.

Long story short, I think this is the correct fix 😄

codetheweb reacted with heart emoji

@novemberborn
Copy link
Member

I'll try and release this today or tomorrow.

1 similar comment
@novemberborn
Copy link
Member

I'll try and release this today or tomorrow.

codetheweb reacted with thumbs up emoji

@novemberbornnovemberborn changed the titleFix shared worker deregistration when global timeout is triggeredUnref shared worker when all test workers have deregisteredDec 13, 2022
@novemberbornnovemberborn merged commit639b905 intoavajs:mainDec 13, 2022
@codetheweb
Copy link
ContributorAuthor

Just a heads up that this seems to cause another bug in our main repo. Still investigating and I haven't been able to reproduce in AVA yet, but might want to hold off on releasing this for a few days--sorry!

gr2m referenced this pull request in gr2m/github-projectDec 5, 2023
[![MendRenovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)This PR contains the following updates:| Package | Change | Age | Adoption | Passing | Confidence ||---|---|---|---|---|---|| [ava](https://avajs.dev) ([source](https://togithub.com/avajs/ava)) |[`^5.0.0` ->`^6.0.0`](https://renovatebot.com/diffs/npm/ava/5.0.1/6.0.0) |[![age](https://developer.mend.io/api/mc/badges/age/npm/ava/6.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/ava/6.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/ava/5.0.1/6.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/ava/5.0.1/6.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)|---### Release Notes<details><summary>avajs/ava (ava)</summary>### [`v6.0.0`](https://togithub.com/avajs/ava/releases/tag/v6.0.0)[Compare Source](https://togithub.com/avajs/ava/compare/v5.3.1...v6.0.0)#### Breaking Changes- AVA now requires Node.js versions 18.18, 20.8 or 21. Versions 14 and16 are no longer supported.[#&#8203;3251](https://togithub.com/avajs/ava/issues/3251)[#&#8203;3216](https://togithub.com/avajs/ava/issues/3216)- When tests finish, worker threads or child processes are no longerexited through `proces.exit()`. If your test file does not exit on itsown, the test run will time out.[#&#8203;3260](https://togithub.com/avajs/ava/issues/3260)- Changes to watch mode[#&#8203;3218](https://togithub.com/avajs/ava/issues/3218):- Watch mode can no longer be started via the `ava.config.*` or`package.json` configuration.- The `ignoredByWatcher` configuration has moved to the `watchMode`object, under the `ignoreChanges` key.- Watch mode now uses the built-in[`fs.watch()`](https://nodejs.org/api/fs.html#fswatchfilename-options-listener)in recursive mode. This is supported on Linux in Node.js 20 or newer,and MacOS and Windows in Node.js 18 as well. There are[caveats](https://nodejs.org/api/fs.html#caveats) to keep in mind.- Failed assertions now throw, meaning that any subsequent code is notexecuted. This also impacts the type definitions.[#&#8203;3246](https://togithub.com/avajs/ava/issues/3246)- [Only nativeerrors](https://nodejs.org/api/util.html#utiltypesisnativeerrorvalue)are now considered errors by the `t.throws()` and `t.throwsAsync()`assertions. [`Object.create(Error.prototype)` is **not** a nativeerror](Object.create\(Error.prototype\)).[#&#8203;3229](https://togithub.com/avajs/ava/issues/3229)- Changes to modules loaded through the `require` configuration[#&#8203;3184](https://togithub.com/avajs/ava/issues/3184):- If such modules export a default function, this function is nowinvoked.    -   Local files are loaded through `@ava/typescript` if necessary.#### Improvements##### Rewritten watcherThe watcher has been rewritten. It’s now built on[`fs.watch()`](https://nodejs.org/api/fs.html#fswatchfilename-options-listener)in recursive mode.[`@vercel/nft`](https://togithub.com/vercel/nft) is used to performstatic dependency analysis, supporting ESM and CJS imports forJavaScript & TypeScript source files. This is a huge improvement overthe previous runtime tracking of CJS imports, which did not support ESM.Integration with[`@ava/typescript`](https://togithub.com/avajs/typescript) has beenimproved. The watcher can now detect a change to a TypeScript sourcefile, then wait for the corresponding build output to change beforere-running tests.The ignoredByWatcher configuration has moved to the watchMode object,under the ignoreChanges key.See [#&#8203;3218](https://togithub.com/avajs/ava/issues/3218) and[#&#8203;3257](https://togithub.com/avajs/ava/issues/3257).##### Failed assertions now throwAssertions now throw a `TestFailure` error when they fail. This error isnot exported or documented and should not be used or thrown manually.You cannot catch this error in order to recover from a failure, use`t.try()` instead.All assertions except for `t.throws()` and `t.throwsAsync()` now return`true` when they pass. This is useful for some of the assertions inTypeScript where they can be used as a type guard.Committing a failed `t.try()` result now also throws.See [#&#8203;3246](https://togithub.com/avajs/ava/issues/3246).##### `t.throws()` and `t.throwsAsync()` can now expect any errorBy default, the thrown error (or rejection reason) must be a nativeerror. You can change the assertion to expect any kind of error bysetting `any: true` in the expectation object:```jst.throws(() => { throw 'error' }, {any: true})```See [#&#8203;3245](https://togithub.com/avajs/ava/issues/3245) by[@&#8203;adiSuper94](https://togithub.com/adiSuper94).##### The `require` configuration is now more powerfulIt now loads ES modules.Local files are loaded through `@ava/typescript` if necessary, so youcan also write these in TypeScript.If there is a default export function, it is invoked after loading. Thefunction is awaited so it can do asynchronous setup before furthermodules are loaded. Arguments from the configuration can be passed tothe function (as a \[[structuredclone](https://developer.mozilla.org/en-US/docs/Web/API/structuredClone)]\(https://developer.mozilla.org/en-US/docs/Web/API/structuredClone)).See [#&#8203;3184](https://togithub.com/avajs/ava/issues/3184) by[@&#8203;sculpt0r](https://togithub.com/sculpt0r).##### Other changes worth noting- Internal events can now be observed (experimentally). See[#&#8203;3247](https://togithub.com/avajs/ava/issues/3247) by[@&#8203;codetheweb](https://togithub.com/codetheweb). It’s experimentaland undocumented.- You can now use `t.timeout.clear()` to restore a previous`t.timeout()`.[#&#8203;3221](https://togithub.com/avajs/ava/issues/3221)- Code coverage is flushed to disk at opportune moments.[#&#8203;3220](https://togithub.com/avajs/ava/issues/3220)#### New Contributors- [@&#8203;sculpt0r](https://togithub.com/sculpt0r) made their firstcontribution in[https://github.com/avajs/ava/pull/3184](https://togithub.com/avajs/ava/pull/3184)- [@&#8203;ZachHaber](https://togithub.com/ZachHaber) made their firstcontribution in[https://github.com/avajs/ava/pull/3233](https://togithub.com/avajs/ava/pull/3233)- [@&#8203;adiSuper94](https://togithub.com/adiSuper94) made their firstcontribution in[https://github.com/avajs/ava/pull/3245](https://togithub.com/avajs/ava/pull/3245)- [@&#8203;bricker](https://togithub.com/bricker) made their firstcontribution in[https://github.com/avajs/ava/pull/3250](https://togithub.com/avajs/ava/pull/3250)**Full Changelog**:avajs/ava@v5.3.1...v6.0.0### [`v5.3.1`](https://togithub.com/avajs/ava/releases/tag/v5.3.1)[Compare Source](https://togithub.com/avajs/ava/compare/v5.3.0...v5.3.1)#### What's Changed- Update `t.like()` to support Symbol keys and ignore non-enumerableproperties by [@&#8203;gibson042](https://togithub.com/gibson042) in[https://github.com/avajs/ava/pull/3209](https://togithub.com/avajs/ava/pull/3209)- Fix circular selector detection in `t.like()` by[@&#8203;novemberborn](https://togithub.com/novemberborn) in[https://github.com/avajs/ava/pull/3212](https://togithub.com/avajs/ava/pull/3212)**Full Changelog**:avajs/ava@v5.3.0...v5.3.1### [`v5.3.0`](https://togithub.com/avajs/ava/releases/tag/v5.3.0)[Compare Source](https://togithub.com/avajs/ava/compare/v5.2.0...v5.3.0)#### What's Changed- Support arrays in `t.like()` assertions by[@&#8203;tommy-mitchell](https://togithub.com/tommy-mitchell) in[https://github.com/avajs/ava/pull/3185](https://togithub.com/avajs/ava/pull/3185)- Recognize typical assertion errors (`expect` and `assert`) and usetheir formatting by [@&#8203;Irvenae](https://togithub.com/Irvenae) in[https://github.com/avajs/ava/pull/3187](https://togithub.com/avajs/ava/pull/3187)#### New Contributors- [@&#8203;MartynasZilinskas](https://togithub.com/MartynasZilinskas)made their first contribution in[https://github.com/avajs/ava/pull/3172](https://togithub.com/avajs/ava/pull/3172)- [@&#8203;flovogt](https://togithub.com/flovogt) made their firstcontribution in[https://github.com/avajs/ava/pull/3194](https://togithub.com/avajs/ava/pull/3194)- [@&#8203;ondreian](https://togithub.com/ondreian) made their firstcontribution in[https://github.com/avajs/ava/pull/3192](https://togithub.com/avajs/ava/pull/3192)- [@&#8203;tommy-mitchell](https://togithub.com/tommy-mitchell) madetheir first contribution in[https://github.com/avajs/ava/pull/3185](https://togithub.com/avajs/ava/pull/3185)- [@&#8203;craigahobbs](https://togithub.com/craigahobbs) made theirfirst contribution in[https://github.com/avajs/ava/pull/3198](https://togithub.com/avajs/ava/pull/3198)- [@&#8203;Irvenae](https://togithub.com/Irvenae) made their firstcontribution in[https://github.com/avajs/ava/pull/3187](https://togithub.com/avajs/ava/pull/3187)**Full Changelog**:avajs/ava@v5.2.0...v5.3.0### [`v5.2.0`](https://togithub.com/avajs/ava/releases/tag/v5.2.0)[Compare Source](https://togithub.com/avajs/ava/compare/v5.1.1...v5.2.0)#### What's Changed- Infer thrown error from expectations by[@&#8203;tao-cumplido](https://togithub.com/tao-cumplido) in[https://github.com/avajs/ava/pull/3156](https://togithub.com/avajs/ava/pull/3156)#### New Contributors- [@&#8203;tao-cumplido](https://togithub.com/tao-cumplido) made theirfirst contribution in[https://github.com/avajs/ava/pull/3156](https://togithub.com/avajs/ava/pull/3156)**Full Changelog**:avajs/ava@v5.1.1...v5.2.0### [`v5.1.1`](https://togithub.com/avajs/ava/releases/tag/v5.1.1)[Compare Source](https://togithub.com/avajs/ava/compare/v5.1.0...v5.1.1)#### What's Changed- Fix de-registration of shared workers to ensure AVA exits correctly,by [@&#8203;codetheweb](https://togithub.com/codetheweb) in[https://github.com/avajs/ava/pull/3149](https://togithub.com/avajs/ava/pull/3149)&[https://github.com/avajs/ava/pull/3151](https://togithub.com/avajs/ava/pull/3151)**Full Changelog**:avajs/ava@v5.1.0...v5.1.1### [`v5.1.0`](https://togithub.com/avajs/ava/releases/tag/v5.1.0)[Compare Source](https://togithub.com/avajs/ava/compare/v5.0.1...v5.1.0)##### What's Changed- Output logs for tests that remain pending when AVA exits by[@&#8203;kevo1ution](https://togithub.com/kevo1ution) in[https://github.com/avajs/ava/pull/3125](https://togithub.com/avajs/ava/pull/3125)- Check for --config file extensions after they fail to load, allowingcustom loaders by [@&#8203;panva](https://togithub.com/panva) in[https://github.com/avajs/ava/pull/3135](https://togithub.com/avajs/ava/pull/3135)##### New Contributors- [@&#8203;kevo1ution](https://togithub.com/kevo1ution) made their firstcontribution in[https://github.com/avajs/ava/pull/3125](https://togithub.com/avajs/ava/pull/3125)- [@&#8203;panva](https://togithub.com/panva) made their firstcontribution in[https://github.com/avajs/ava/pull/3135](https://togithub.com/avajs/ava/pull/3135)**Full Changelog**:avajs/ava@v5.0.1...v5.1.0</details>---### Configuration📅 **Schedule**: Branch creation - At any time (no schedule defined),Automerge - At any time (no schedule defined).🚦 **Automerge**: Disabled by config. Please merge this manually once youare satisfied.♻ **Rebasing**: Whenever PR becomes conflicted, or you tick therebase/retry checkbox.🔕 **Ignore**: Close this PR and you won't be reminded about this updateagain.---- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, checkthis box---This PR has been generated by [MendRenovate](https://www.mend.io/free-developer-tools/renovate/). Viewrepository job log[here](https://developer.mend.io/github/gr2m/github-project).<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy44MS4zIiwidXBkYXRlZEluVmVyIjoiMzcuODEuMyIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@codetheweb@novemberborn

[8]ページ先頭

©2009-2025 Movatter.jp