-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Check failed: !holder_map.has_named_interceptor().
when running jest
#40030
Comments
I'm sad I did not see that one, because I looked at the result for other machines and it was related to MongoDB process issues as usual... Here's another run with the same failure against the master branch: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2756/nodes=ubuntu1804-64/testReport/junit/(root)/citgm/jest_v27_1_0/ /cc @nodejs/v8 |
I think the failing check is from v8/v8@76b9d98 /cc @schuay |
The following can be used to reproduce this bug (in my case it crashes at 140): describe('holder_map.has_named_interceptor crash', () => {
for (let i = 0; i < 140; i++) {
it(`${i}`, () => {
//
});
}
}); Run test: jest test.js |
The bug is triggered by the compilation of this function: |
Repro without Jest: const vm = require('vm');
vm.runInNewContext(`
const STATE_SYM = Symbol('JEST_STATE_SYMBOL')
const eventHandlers = [() => {}];
const dispatch = async () => {
for (const handler of eventHandlers) {
await handler(this[STATE_SYM]);
}
};
for (let i = 0; i < 1e6; i++) {
dispatch();
}
`); |
I created an upstream bug: v8:12188 |
Is this caused by a v8 update? If so, any chance of reverting it? Or do we think (hope 😀) a fix will land upstream relatively quickly and it'll be cherry picked?
We should skip the mongo test if it causes noise hiding real issues. I'll open an issue in CITGM to track EDIT: nodejs/citgm#875 |
I expect we'll fix this upstream in the next couple of days. |
@SimenB In the mean time, would you be open to tweaking the problematic function in |
Happy to land any change that would prevent it or just make it better in general and avoiding the problem altogether 🙂 |
nodejs/node#40030 Bug found in Node js; removed 16x for now
Jest tests are failing on Github actions also for the same reason like reproduction script after using
|
Due to hitting this error: nodejs/node#40030
Bug nodejs/node#40030 will be fixed in Node.js v16.9.1: https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V16.md#2021-09-10-version-1691-current-richardlau This bug has been causing frequent spurious test failures for Apollo Client. A few examples: https://app.circleci.com/pipelines/github/apollographql/apollo-client/11547/workflows/4027b7e7-b8bd-4cf1-8927-f08921a4c577/jobs/83124 https://app.circleci.com/pipelines/github/apollographql/apollo-client/11571/workflows/3e78ba11-0e51-4f4f-92dd-e732c53716d2/jobs/83167 https://app.circleci.com/pipelines/github/apollographql/apollo-client/11536/workflows/55d29d9f-b4a7-4803-80bb-e3891946960d/jobs/83106 CircleCI ships docker containers for most Node.js versions, but they haven't shipped a new version for 16.9.1 yet (16.9.0 is the latest). We will switch back to `cimg/node:16` once that gives us a version greater than or equal to 16.9.1.
Original commit message: [compiler] Gracefully handle an unsupported situation ... by skipping the optimization instead of CHECK-failing. Bug: v8:12188 Change-Id: I6709bf1c55506f3d12886efbfbb9934788cd02ce Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3148132 Auto-Submit: Georg Neis <neis@chromium.org> Commit-Queue: Jakob Gruber <jgruber@chromium.org> Reviewed-by: Jakob Gruber <jgruber@chromium.org> Cr-Commit-Position: refs/heads/main@{#76741} Refs: v8/v8@9a60704 PR-URL: nodejs#40046 Fixes: nodejs#40030 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>
Original commit message: [compiler] Gracefully handle an unsupported situation ... by skipping the optimization instead of CHECK-failing. Bug: v8:12188 Change-Id: I6709bf1c55506f3d12886efbfbb9934788cd02ce Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3148132 Auto-Submit: Georg Neis <neis@chromium.org> Commit-Queue: Jakob Gruber <jgruber@chromium.org> Reviewed-by: Jakob Gruber <jgruber@chromium.org> Cr-Commit-Position: refs/heads/main@{#76741} Refs: v8/v8@9a60704 PR-URL: nodejs#40046 Fixes: nodejs#40030 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>
Original commit message: [compiler] Gracefully handle an unsupported situation ... by skipping the optimization instead of CHECK-failing. Bug: v8:12188 Change-Id: I6709bf1c55506f3d12886efbfbb9934788cd02ce Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3148132 Auto-Submit: Georg Neis <neis@chromium.org> Commit-Queue: Jakob Gruber <jgruber@chromium.org> Reviewed-by: Jakob Gruber <jgruber@chromium.org> Cr-Commit-Position: refs/heads/main@{#76741} Refs: v8/v8@9a60704 PR-URL: nodejs#40046 Fixes: nodejs#40030 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>
Original commit message: [compiler] Gracefully handle an unsupported situation ... by skipping the optimization instead of CHECK-failing. Bug: v8:12188 Change-Id: I6709bf1c55506f3d12886efbfbb9934788cd02ce Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3148132 Auto-Submit: Georg Neis <neis@chromium.org> Commit-Queue: Jakob Gruber <jgruber@chromium.org> Reviewed-by: Jakob Gruber <jgruber@chromium.org> Cr-Commit-Position: refs/heads/main@{#76741} Refs: v8/v8@9a60704 PR-URL: #40046 Fixes: #40030 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>
Original commit message: [compiler] Gracefully handle an unsupported situation ... by skipping the optimization instead of CHECK-failing. Bug: v8:12188 Change-Id: I6709bf1c55506f3d12886efbfbb9934788cd02ce Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3148132 Auto-Submit: Georg Neis <neis@chromium.org> Commit-Queue: Jakob Gruber <jgruber@chromium.org> Reviewed-by: Jakob Gruber <jgruber@chromium.org> Cr-Commit-Position: refs/heads/main@{#76741} Refs: v8/v8@9a60704 PR-URL: nodejs#40046 Fixes: nodejs#40030 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>
Original commit message: [compiler] Gracefully handle an unsupported situation ... by skipping the optimization instead of CHECK-failing. Bug: v8:12188 Change-Id: I6709bf1c55506f3d12886efbfbb9934788cd02ce Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3148132 Auto-Submit: Georg Neis <neis@chromium.org> Commit-Queue: Jakob Gruber <jgruber@chromium.org> Reviewed-by: Jakob Gruber <jgruber@chromium.org> Cr-Commit-Position: refs/heads/main@{#76741} Refs: v8/v8@9a60704 PR-URL: nodejs#40046 Fixes: nodejs#40030 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>
…ad/download without saving to disk (#3159) * rewrite to async/await * Only fetch size (HEAD) if needed #3034 * Update packages/@uppy/companion/src/server/controllers/url.js Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> * Change HEAD to GET in getURLMeta and abort request immediately upon response headers received #3034 (comment) * fix lint * fix lint * cut off length of file names or else we get "MetadataTooLarge: Your metadata headers exceed the maximum allowed metadata size" in tus / S3 * try to fix flaky test * remove iife and cleanup code a bit * fix lint by reordering code * rename Uploader to MultipartUploader * Rewrite Uploader to use fs-capacitor #3098 This allows for upload to start almost immediately without having to first download the file. And it allows for uploading bigger files, because transloadit assembly will not timeout, as it will get upload progress events all the time. No longer need for illusive progress. Also fix eslint warnings and simplify logic Still TODO: TUS pause/resume has a bug: tus/tus-js-client#275 * add comment in dev Dashboard and pull out variable * fix a bug where remote xhr upload would ignore progress events in the UI * fix bug where s3 multipart cancel wasn't working * fix also cancel for xhr * Rewrite providers to use streams This removes the need for disk space as data will be buffered in memory and backpressure will be respected #3098 (comment) All providers "download" methods will now return a { stream } which can be consumed by uploader. Also: - Remove capacitor (no longer needed) - Change Provider/SearchProvider API to async (Breaking change for custom companion providers) - Fix the case with unknown length streams (zoom / google drive). Need to be downloaded first - rewrite controllers deauth-callback, thumbnail, list, logout to async - getURLMeta: make sure size is never NaN (NaN gets converted to null in JSON.stringify when sent to client but not when used in backend) - fix purest mock (it wasn't returning statusCode on('response')) - add missing http mock for "request" for THUMBNAIL_URL and http://url.myendpoint.com/file (these request errors were never caught by tests previously) - "upload functions with tus protocol" test: move filename checking to new test where size is null. Fix broken expects - fix some lint * Implement streamingUpload flag COMPANION_STREAMING_UPLOAD Default to false due to backward compatibility If set to true, will start to upload files at the same time as dowlnoading them, by piping the streams - Also implement progress for downloading too - and fix progress duplication logic - fix test that assumed file was fully downloaded after first progress event * rearrange validation logic * add COMPANION_STREAMING_UPLOAD to env.test.sh too * implement maxFileSize option in companion for both unknown length and known length downloads * fix bug * fix memory leak when non 200 status streams were being kept * fix lint * Add backward-compatibility for companion providers Implement a new static field "version" on providers, which when not set to 2, will cause a compatibility layer to be added for supporting old callback style provider api also fix some eslint and rename some vars * document new provider API * remove static as it doesn't work on node 10 * try to fix build issue * degrade to node 14 in github actions due to hitting this error: nodejs/node#40030 https://github.com/transloadit/uppy/pull/3159/checks?check_run_id=3544858518 * pull out duplicated logic into reusable function * fix lint * make methods private * re-add unsplash download_location request got lost in merge * add try/catch as suggested #3159 (comment) * Only set default chunkSize if needed for being more compliant with previous behavior when streamingUpload = false * Improve flaky test Trying to fix this error: FAIL packages/@uppy/utils/src/delay.test.js ● delay › should reject when signal is aborted expect(received).toBeLessThan(expected) Expected: < 70 Received: 107 32 | const time = Date.now() - start 33 | expect(time).toBeGreaterThanOrEqual(30) > 34 | expect(time).toBeLessThan(70) | ^ 35 | }) 36 | }) 37 | at Object.<anonymous> (packages/@uppy/utils/src/delay.test.js:34:18) https://github.com/transloadit/uppy/runs/3984613454?check_suite_focus=true * Apply suggestions from code review Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> * fix review feedback & lint * Apply suggestions from code review Co-authored-by: Merlijn Vos <merlijn@soverin.net> * remove unneeded ts-ignore * Update packages/@uppy/companion/src/server/controllers/url.js Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> * Update packages/@uppy/companion/src/server/Uploader.js Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> * reduce nesting * fix lint * optimize promisify #3159 (comment) * Update packages/@uppy/companion/test/__tests__/uploader.js Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> Co-authored-by: Merlijn Vos <merlijn@soverin.net>
- Addjust codec types to support protocol version 0.12. - Pin node 16 version because the latest node 16 breaks jest nodejs/node#40030
Unpin node 16 from 16.8 in the github test workflow because a fix for nodejs/node#40030 has been released.
…ad/download without saving to disk (transloadit#3159) * rewrite to async/await * Only fetch size (HEAD) if needed transloadit#3034 * Update packages/@uppy/companion/src/server/controllers/url.js Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> * Change HEAD to GET in getURLMeta and abort request immediately upon response headers received transloadit#3034 (comment) * fix lint * fix lint * cut off length of file names or else we get "MetadataTooLarge: Your metadata headers exceed the maximum allowed metadata size" in tus / S3 * try to fix flaky test * remove iife and cleanup code a bit * fix lint by reordering code * rename Uploader to MultipartUploader * Rewrite Uploader to use fs-capacitor transloadit#3098 This allows for upload to start almost immediately without having to first download the file. And it allows for uploading bigger files, because transloadit assembly will not timeout, as it will get upload progress events all the time. No longer need for illusive progress. Also fix eslint warnings and simplify logic Still TODO: TUS pause/resume has a bug: tus/tus-js-client#275 * add comment in dev Dashboard and pull out variable * fix a bug where remote xhr upload would ignore progress events in the UI * fix bug where s3 multipart cancel wasn't working * fix also cancel for xhr * Rewrite providers to use streams This removes the need for disk space as data will be buffered in memory and backpressure will be respected transloadit#3098 (comment) All providers "download" methods will now return a { stream } which can be consumed by uploader. Also: - Remove capacitor (no longer needed) - Change Provider/SearchProvider API to async (Breaking change for custom companion providers) - Fix the case with unknown length streams (zoom / google drive). Need to be downloaded first - rewrite controllers deauth-callback, thumbnail, list, logout to async - getURLMeta: make sure size is never NaN (NaN gets converted to null in JSON.stringify when sent to client but not when used in backend) - fix purest mock (it wasn't returning statusCode on('response')) - add missing http mock for "request" for THUMBNAIL_URL and http://url.myendpoint.com/file (these request errors were never caught by tests previously) - "upload functions with tus protocol" test: move filename checking to new test where size is null. Fix broken expects - fix some lint * Implement streamingUpload flag COMPANION_STREAMING_UPLOAD Default to false due to backward compatibility If set to true, will start to upload files at the same time as dowlnoading them, by piping the streams - Also implement progress for downloading too - and fix progress duplication logic - fix test that assumed file was fully downloaded after first progress event * rearrange validation logic * add COMPANION_STREAMING_UPLOAD to env.test.sh too * implement maxFileSize option in companion for both unknown length and known length downloads * fix bug * fix memory leak when non 200 status streams were being kept * fix lint * Add backward-compatibility for companion providers Implement a new static field "version" on providers, which when not set to 2, will cause a compatibility layer to be added for supporting old callback style provider api also fix some eslint and rename some vars * document new provider API * remove static as it doesn't work on node 10 * try to fix build issue * degrade to node 14 in github actions due to hitting this error: nodejs/node#40030 https://github.com/transloadit/uppy/pull/3159/checks?check_run_id=3544858518 * pull out duplicated logic into reusable function * fix lint * make methods private * re-add unsplash download_location request got lost in merge * add try/catch as suggested transloadit#3159 (comment) * Only set default chunkSize if needed for being more compliant with previous behavior when streamingUpload = false * Improve flaky test Trying to fix this error: FAIL packages/@uppy/utils/src/delay.test.js ● delay › should reject when signal is aborted expect(received).toBeLessThan(expected) Expected: < 70 Received: 107 32 | const time = Date.now() - start 33 | expect(time).toBeGreaterThanOrEqual(30) > 34 | expect(time).toBeLessThan(70) | ^ 35 | }) 36 | }) 37 | at Object.<anonymous> (packages/@uppy/utils/src/delay.test.js:34:18) https://github.com/transloadit/uppy/runs/3984613454?check_suite_focus=true * Apply suggestions from code review Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> * fix review feedback & lint * Apply suggestions from code review Co-authored-by: Merlijn Vos <merlijn@soverin.net> * remove unneeded ts-ignore * Update packages/@uppy/companion/src/server/controllers/url.js Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> * Update packages/@uppy/companion/src/server/Uploader.js Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> * reduce nesting * fix lint * optimize promisify transloadit#3159 (comment) * Update packages/@uppy/companion/test/__tests__/uploader.js Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> Co-authored-by: Merlijn Vos <merlijn@soverin.net>
Version
16.9.0
Platform
Linux oliver-home 5.10.16.3-microsoft-standard-WSL2 #1 SMP Fri Apr 2 22:23:49 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Subsystem
vm
What steps will reproduce the bug?
#40030 (comment)
How often does it reproduce? Is there a required condition?
Every single time.
What is the expected behavior?
Tests run as usual.
What do you see instead?
Additional information
#13804 sounds very related
16.8.0 didn't produce this result and still works as expected.
The text was updated successfully, but these errors were encountered: