Skip to content

Esm off thread unlock worker #2

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.

Sign up for GitHub

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

Jump to bottom
Merged
GeoffreyBooth merged 3 commits into from
Jan 9, 2023
Merged

Esm off thread unlock worker #2

GeoffreyBooth merged 3 commits into from
Jan 9, 2023

Conversation

@targos
Copy link

@targos targos commented Jan 1, 2023

I compared the test results before and after the changes (deleted identical output).

Before (16 failures) 
=== release test-esm-loader ===
Path: es-module/test-esm-loader
(node:3020513) ExperimentalWarning: Custom ESM Loaders is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
node:internal/process/esm_loader:19
    internalBinding('errors').triggerUncaughtException(
                              ^

RangeError [ERR_OUT_OF_RANGE]: The value of "serializedResponse.byteLength" is out of range. It must be <= 2048. Received 2651
    at new NodeError (node:internal/errors:399:5)
    at setupESMWorker (node:internal/modules/esm/worker:80:15)

Node.js v20.0.0-pre
Command: out/Release/node --experimental-loader ./test/fixtures/es-module-loaders/hooks-custom.mjs /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader.mjs


=== release test-esm-loader-stringify-text ===
Path: es-module/test-esm-loader-stringify-text
(node:3020959) ExperimentalWarning: Custom ESM Loaders is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
node:internal/process/esm_loader:19
    internalBinding('errors').triggerUncaughtException(
                              ^

RangeError [ERR_OUT_OF_RANGE]: The value of "serializedResponse.byteLength" is out of range. It must be <= 2048. Received 2511
    at new NodeError (node:internal/errors:399:5)
    at setupESMWorker (node:internal/modules/esm/worker:80:15)

Node.js v20.0.0-pre
Command: out/Release/node --experimental-loader ./test/fixtures/es-module-loaders/string-sources.mjs /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-stringify-text.mjs


=== release test-esm-named-exports ===
Path: es-module/test-esm-named-exports
(node:3021177) ExperimentalWarning: Custom ESM Loaders is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
node:internal/process/esm_loader:19
    internalBinding('errors').triggerUncaughtException(
                              ^

RangeError [ERR_OUT_OF_RANGE]: The value of "serializedResponse.byteLength" is out of range. It must be <= 2048. Received 5398
    at new NodeError (node:internal/errors:399:5)
    at setupESMWorker (node:internal/modules/esm/worker:80:15)

Node.js v20.0.0-pre
Command: out/Release/node --experimental-loader ./test/fixtures/es-module-loaders/builtin-named-exports-loader.mjs /home/mzasso/git/nodejs/node/test/es-module/test-esm-named-exports.mjs


=== release test-esm-loader-obsolete-hooks ===
Path: es-module/test-esm-loader-obsolete-hooks
TAP version 13
# Subtest: ESM: deprecation warnings for obsolete hooks
    # Subtest: <anonymous>
    not ok 1 - <anonymous>
      ---
      duration_ms: 223.595027
      failureType: 'testCodeFailure'
      error: |-
        The input did not match the regular expression /DeprecationWarning:/. Input:

        'Bad command or file name\n'

      code: 'ERR_ASSERTION'
      actual: |-
        Bad command or file name

      operator: 'match'
      stack: |-
        Object.<anonymous> (file:///home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-obsolete-hooks.mjs:20:5)
        async ItTest.run (node:internal/test_runner/test:548:9)
        async Promise.all (index 0)
        async Suite.run (node:internal/test_runner/test:798:7)
      ...
    1..1
not ok 1 - ESM: deprecation warnings for obsolete hooks
  ---
  duration_ms: 226.322374
  failureType: 'subtestsFailed'
  error: '1 subtest failed'
  code: 'ERR_TEST_FAILURE'
  ...
1..1
# tests 1
# pass 0
# fail 1
# cancelled 0
# skipped 0
# todo 0
# duration_ms 232.123419
Command: out/Release/node /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-obsolete-hooks.mjs


=== release test-esm-loader-chaining ===
Path: es-module/test-esm-loader-chaining
TAP version 13
# Subtest: ESM: loader chaining
    # Subtest: should load unadulterated source when there are no loaders
    ok 1 - should load unadulterated source when there are no loaders
      ---
      duration_ms: 295.432017
      ...
    # Subtest: should load properly different source when only load changes something
    ok 2 - should load properly different source when only load changes something
      ---
      duration_ms: 292.425237
      ...
    # Subtest: should result in proper output from multiple changes in resolve hooks
    ok 3 - should result in proper output from multiple changes in resolve hooks
      ---
      duration_ms: 289.996562
      ...
    # Subtest: should respect modified context within resolve chain
    ok 4 - should respect modified context within resolve chain
      ---
      duration_ms: 286.858653
      ...
    # Subtest: should accept only the correct arguments
    not ok 5 - should accept only the correct arguments
      ---
      duration_ms: 337.407822
      failureType: 'testCodeFailure'
      error: |-
        The input did not match the regular expression /specifier: 'node:fs'/. Input:

        'resolve arg count: 3\n' +
          '{"specifier":"node:fs","context":{"conditions":["node","import","node-addons"],"importAssertions":{},"parentURL":"file:///home/mzasso/git/nodejs/node/[eval1]"}}\n' +
          'load arg count: 3{"url":"node:fs","context":{"importAssertions":{}}}\n' +
          'resolve arg count: 3\n' +
          '{"specifier":"node:fs","context":{"conditions":["node","import","node-addons"],"importAssertions":{},"parentURL":"file:///home/mzasso/git/nodejs/node/[eval1]"}}\n' +
          'load arg count: 3{"url":"node:fs","context":{}}\n'

      code: 'ERR_ASSERTION'
      actual: |-
        resolve arg count: 3
        {"specifier":"node:fs","context":{"conditions":["node","import","node-addons"],"importAssertions":{},"parentURL":"file:///home/mzasso/git/nodejs/node/[eval1]"}}
        load arg count: 3{"url":"node:fs","context":{"importAssertions":{}}}
        resolve arg count: 3
        {"specifier":"node:fs","context":{"conditions":["node","import","node-addons"],"importAssertions":{},"parentURL":"file:///home/mzasso/git/nodejs/node/[eval1]"}}
        load arg count: 3{"url":"node:fs","context":{}}

      operator: 'match'
      stack: |-
        Object.<anonymous> (file:///home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-chaining.mjs:117:12)
        async ItTest.run (node:internal/test_runner/test:548:9)
        async Promise.all (index 4)
        async Suite.run (node:internal/test_runner/test:798:7)
      ...
    # Subtest: should result in proper output from multiple changes in resolve hooks
    ok 6 - should result in proper output from multiple changes in resolve hooks
      ---
      duration_ms: 287.057979
      ...
    # Subtest: should provide the correct "next" fn when multiple calls to next within same loader
    ok 7 - should provide the correct "next" fn when multiple calls to next within same loader
      ---
      duration_ms: 269.567886
      ...
    # Subtest: should use the correct `name` for next<HookName>'s function
    not ok 8 - should use the correct `name` for next<HookName>'s function
      ---
      duration_ms: 345.105218
      failureType: 'testCodeFailure'
      error: |-
        The input did not match the regular expression /next<HookName>: nextResolve/. Input:

        'resolve 42\nnext<HookName>:nextResolve\n42\n'

      code: 'ERR_ASSERTION'
      actual: |-
        resolve 42
        next<HookName>:nextResolve
        42

      operator: 'match'
      stack: |-
        Object.<anonymous> (file:///home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-chaining.mjs:188:12)
        process.processTicksAndRejections (node:internal/process/task_queues:95:5)
        async ItTest.run (node:internal/test_runner/test:548:9)
        async Promise.all (index 7)
        async Suite.run (node:internal/test_runner/test:798:7)
      ...
    # Subtest: should throw for incomplete resolve chain, citing errant loader & hook
    ok 9 - should throw for incomplete resolve chain, citing errant loader & hook
      ---
      duration_ms: 262.020809
      ...
    # Subtest: should NOT throw when nested resolve hook signaled a short circuit
    ok 10 - should NOT throw when nested resolve hook signaled a short circuit
      ---
      duration_ms: 403.642323
      ...
    # Subtest: should NOT throw when nested load hook signaled a short circuit
    ok 11 - should NOT throw when nested load hook signaled a short circuit
      ---
      duration_ms: 355.875815
      ...
    # Subtest: should allow loaders to influence subsequent loader resolutions
    not ok 12 - should allow loaders to influence subsequent loader resolutions
      ---
      duration_ms: 307.344266
      failureType: 'testCodeFailure'
      error: |-
        Expected values to be strictly equal:
        + actual - expected

        + 'node:internal/process/esm_loader:19\n' +
        +   "    internalBinding('errors').triggerUncaughtException(\n" +
        +   '                              ^\n' +
        +   '\n' +
        +   "Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'xxx' imported from /home/mzasso/git/nodejs/node/test/fixtures/es-module-loaders/\n" +
        +   '    at new NodeError (node:internal/errors:399:5)\n' +
        +   '    at packageResolve (node:internal/modules/esm/resolve:795:9)\n' +
        +   '    at moduleResolve (node:internal/modules/esm/resolve:844:20)\n' +
        +   '    at DefaultModuleLoader.defaultResolve (node:internal/modules/esm/resolve:1069:11)\n' +
        +   '    at DefaultModuleLoader.resolve (node:internal/modules/esm/loader:298:32)\n' +
        +   '    at DefaultModuleLoader.getModuleJob (node:internal/modules/esm/loader:150:34)\n' +
        +   '    at DefaultModuleLoader.import (node:internal/modules/esm/loader:252:10)\n' +
        +   '    at initializeHooks (node:internal/modules/esm/utils:122:65)\n' +
        +   '    at async setupESMWorker (node:internal/modules/esm/worker:44:13)\n' +
        +   '\n' +
        +   'Node.js v20.0.0-pre\n'
        - ''
      code: 'ERR_ASSERTION'
      expected: ''
      actual: |-
        node:internal/process/esm_loader:19
            internalBinding('errors').triggerUncaughtException(
                                      ^

        Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'xxx' imported from /home/mzasso/git/nodejs/node/test/fixtures/es-module-loaders/
            at new NodeError (node:internal/errors:399:5)
            at packageResolve (node:internal/modules/esm/resolve:795:9)
            at moduleResolve (node:internal/modules/esm/resolve:844:20)
            at DefaultModuleLoader.defaultResolve (node:internal/modules/esm/resolve:1069:11)
            at DefaultModuleLoader.resolve (node:internal/modules/esm/loader:298:32)
            at DefaultModuleLoader.getModuleJob (node:internal/modules/esm/loader:150:34)
            at DefaultModuleLoader.import (node:internal/modules/esm/loader:252:10)
            at initializeHooks (node:internal/modules/esm/utils:122:65)
            at async setupESMWorker (node:internal/modules/esm/worker:44:13)

        Node.js v20.0.0-pre

      operator: 'strictEqual'
      stack: |-
        Object.<anonymous> (file:///home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-chaining.mjs:268:12)
        process.processTicksAndRejections (node:internal/process/task_queues:95:5)
        async ItTest.run (node:internal/test_runner/test:548:9)
        async Promise.all (index 11)
        async Suite.run (node:internal/test_runner/test:798:7)
      ...
    # Subtest: should throw when the resolve chain is broken
    ok 13 - should throw when the resolve chain is broken
      ---
      duration_ms: 302.525516
      ...
    # Subtest: should throw for incomplete load chain, citing errant loader & hook
    ok 14 - should throw for incomplete load chain, citing errant loader & hook
      ---
      duration_ms: 273.592653
      ...
    # Subtest: should throw when the load chain is broken
    ok 15 - should throw when the load chain is broken
      ---
      duration_ms: 267.83066
      ...
    # Subtest: should throw when invalid `specifier` argument passed to `nextResolve`
    ok 16 - should throw when invalid `specifier` argument passed to `nextResolve`
      ---
      duration_ms: 261.824183
      ...
    # Subtest: should throw when resolve hook is invalid
    ok 17 - should throw when resolve hook is invalid
      ---
      duration_ms: 255.64576
      ...
    # Subtest: should throw when invalid `context` argument passed to `nextResolve`
    ok 18 - should throw when invalid `context` argument passed to `nextResolve`
      ---
      duration_ms: 291.104702
      ...
    # Subtest: should throw when load hook is invalid
    ok 19 - should throw when load hook is invalid
      ---
      duration_ms: 225.781249
      ...
    # Subtest: should throw when invalid `url` argument passed to `nextLoad`
    ok 20 - should throw when invalid `url` argument passed to `nextLoad`
      ---
      duration_ms: 259.113417
      ...
    # Subtest: should throw when invalid `url` argument passed to `nextLoad`
    ok 21 - should throw when invalid `url` argument passed to `nextLoad`
      ---
      duration_ms: 277.272299
      ...
    # Subtest: should throw when invalid `context` argument passed to `nextLoad`
    ok 22 - should throw when invalid `context` argument passed to `nextLoad`
      ---
      duration_ms: 240.81358
      ...
    1..22
not ok 1 - ESM: loader chaining
  ---
  duration_ms: 527.078898
  failureType: 'subtestsFailed'
  error: '3 subtests failed'
  code: 'ERR_TEST_FAILURE'
  ...
1..1
# tests 1
# pass 0
# fail 1
# cancelled 0
# skipped 0
# todo 0
# duration_ms 531.606638
Command: out/Release/node /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-chaining.mjs


[06:17|% 100|+ 3795|-  16]: Done

Failed tests:
out/Release/node --experimental-loader ./test/fixtures/es-module-loaders/assertionless-json-import.mjs /home/mzasso/git/nodejs/node/test/es-module/test-esm-assertionless-json-import.js
out/Release/node --experimental-loader ./test/fixtures/es-module-loaders/example-loader.mjs /home/mzasso/git/nodejs/node/test/es-module/test-esm-example-loader.mjs
out/Release/node --experimental-import-meta-resolve /home/mzasso/git/nodejs/node/test/es-module/test-esm-import-meta-resolve.mjs
out/Release/node --experimental-loader ./test/fixtures/es-module-loaders/hooks-custom.mjs /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader.mjs
out/Release/node /home/mzasso/git/nodejs/node/test/es-module/test-esm-import-flag.mjs
out/Release/node --loader ./test/fixtures/es-module-loaders/mock-loader.mjs /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-mock.mjs
out/Release/node --experimental-loader ./test/fixtures/es-module-loaders/loader-invalid-url.mjs /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-invalid-url.mjs
out/Release/node --experimental-loader ./test/fixtures/es-module-loaders/loader-side-effect.mjs --require ./test/fixtures/es-module-loaders/loader-side-effect-require-preload.js /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-side-effect.mjs
out/Release/node --experimental-loader ./test/fixtures/es-module-loaders/string-sources.mjs /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-stringify-text.mjs
out/Release/node --loader ./test/fixtures/es-module-loaders/hook-resolve-type.mjs /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-resolve-type.mjs
out/Release/node --experimental-loader ./test/fixtures/es-module-loaders/builtin-named-exports-loader.mjs /home/mzasso/git/nodejs/node/test/es-module/test-esm-named-exports.mjs
out/Release/node /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-obsolete-hooks.mjs
out/Release/node /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-http-imports.mjs
out/Release/node /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-chaining.mjs
out/Release/node /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-thenable.mjs
out/Release/node /home/mzasso/git/nodejs/node/test/es-module/test-esm-nowarn-exports.mjs
make[1]: *** [Makefile:308: jstest] Error 1
make: *** [Makefile:334: test] Error 2
After (18 failures) 
=== release test-esm-loader ===
Path: es-module/test-esm-loader
(node:2968400) ExperimentalWarning: Custom ESM Loaders is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
node:internal/process/esm_loader:19
    internalBinding('errors').triggerUncaughtException(
                              ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected

+ Comparison {}
- Comparison {
-   code: 'ERR_MODULE_NOT_FOUND'
- }
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async file:///home/mzasso/git/nodejs/node/test/es-module/test-esm-loader.mjs:6:1 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'nonexistent' imported from /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader.mjs
      at new NodeError (node:internal/errors:399:5)
      at packageResolve (node:internal/modules/esm/resolve:795:9)
      at moduleResolve (node:internal/modules/esm/resolve:844:20)
      at defaultResolve (node:internal/modules/esm/resolve:1069:11)
      at nextResolve (node:internal/modules/esm/hooks:638:28)
      at resolve (file:///home/mzasso/git/nodejs/node/test/fixtures/es-module-loaders/hooks-custom.mjs:76:10)
      at nextResolve (node:internal/modules/esm/hooks:638:28)
      at Hooks.resolve (node:internal/modules/esm/hooks:244:26)
      at MessagePort.handleSyncMessage (node:internal/modules/esm/worker:56:26)
      at [nodejs.internal.kHybridDispatch] (node:internal/event_target:738:20),
  expected: { code: 'ERR_MODULE_NOT_FOUND' },
  operator: 'rejects'
}

Node.js v20.0.0-pre
Command: out/Release/node --experimental-loader ./test/fixtures/es-module-loaders/hooks-custom.mjs /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader.mjs


=== release test-esm-loader-stringify-text ===
Path: es-module/test-esm-loader-stringify-text
(node:2968958) ExperimentalWarning: Custom ESM Loaders is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
node:internal/process/promises:289
            triggerUncaughtException(err, true /* fromPromise */);
            ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ undefined
- 'ERR_INVALID_RETURN_PROPERTY_VALUE'
    at file:///home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-stringify-text.mjs:8:12
    at /home/mzasso/git/nodejs/node/test/common/index.js:448:15
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: undefined,
  expected: 'ERR_INVALID_RETURN_PROPERTY_VALUE',
  operator: 'strictEqual'
}

Node.js v20.0.0-pre
Command: out/Release/node --experimental-loader ./test/fixtures/es-module-loaders/string-sources.mjs /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-stringify-text.mjs


=== release test-esm-named-exports ===
Path: es-module/test-esm-named-exports
(node:2969120) ExperimentalWarning: Custom ESM Loaders is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
custom-node:module:1
const $builtinInstance = $__get_builtin_hole_1672580803226("module");
                         ^

ReferenceError: $__get_builtin_hole_1672580803226 is not defined
    at custom-node:module:1:26
    at ModuleJob.run (node:internal/modules/esm/module_job:193:25)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

Node.js v20.0.0-pre
Command: out/Release/node --experimental-loader ./test/fixtures/es-module-loaders/builtin-named-exports-loader.mjs /home/mzasso/git/nodejs/node/test/es-module/test-esm-named-exports.mjs


=== release test-esm-loader-obsolete-hooks ===
Path: es-module/test-esm-loader-obsolete-hooks
TAP version 13
Command: out/Release/node /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-obsolete-hooks.mjs
--- TIMEOUT ---


=== release test-esm-loader-chaining ===
Path: es-module/test-esm-loader-chaining
TAP version 13
# Subtest: ESM: loader chaining
    # Subtest: should load unadulterated source when there are no loaders
    ok 1 - should load unadulterated source when there are no loaders
      ---
      duration_ms: 225.710985
      ...
    # Subtest: should load properly different source when only load changes something
    not ok 2 - should load properly different source when only load changes something
      ---
      duration_ms: 222.6427
      failureType: 'testCodeFailure'
      error: |-
        The input did not match the regular expression /load passthru/. Input:

        'foo\nresolve passthru\n'

      code: 'ERR_ASSERTION'
      actual: |-
        foo
        resolve passthru

      operator: 'match'
      stack: |-
        Object.<anonymous> (file:///home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-chaining.mjs:50:12)
        async ItTest.run (node:internal/test_runner/test:548:9)
        async Promise.all (index 1)
        async Suite.run (node:internal/test_runner/test:798:7)
      ...
    # Subtest: should result in proper output from multiple changes in resolve hooks
    ok 3 - should result in proper output from multiple changes in resolve hooks
      ---
      duration_ms: 230.775636
      ...
    # Subtest: should respect modified context within resolve chain
    not ok 4 - should respect modified context within resolve chain
      ---
      duration_ms: 227.725052
      failureType: 'testCodeFailure'
      error: |-
        The input did not match the regular expression /bar/. Input:

        '42\nresolve 42\n'

      code: 'ERR_ASSERTION'
      actual: |-
        42
        resolve 42

      operator: 'match'
      stack: |-
        Object.<anonymous> (file:///home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-chaining.mjs:99:12)
        async ItTest.run (node:internal/test_runner/test:548:9)
        async Promise.all (index 3)
        async Suite.run (node:internal/test_runner/test:798:7)
      ...
    # Subtest: should accept only the correct arguments
    not ok 5 - should accept only the correct arguments
      ---
      duration_ms: 213.329172
      failureType: 'testCodeFailure'
      error: |-
        The input did not match the regular expression /^resolve arg count: 3$/m. Input:

        ''

      code: 'ERR_ASSERTION'
      actual: ''
      operator: 'match'
      stack: |-
        Object.<anonymous> (file:///home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-chaining.mjs:116:12)
        async ItTest.run (node:internal/test_runner/test:548:9)
        async Promise.all (index 4)
        async Suite.run (node:internal/test_runner/test:798:7)
      ...
    # Subtest: should result in proper output from multiple changes in resolve hooks
    ok 6 - should result in proper output from multiple changes in resolve hooks
      ---
      duration_ms: 214.385092
      ...
    # Subtest: should provide the correct "next" fn when multiple calls to next within same loader
    not ok 7 - should provide the correct "next" fn when multiple calls to next within same loader
      ---
      duration_ms: 239.430812
      failureType: 'testCodeFailure'
      error: |-
        Expected values to be strictly equal:

        1 !== 2

      code: 'ERR_ASSERTION'
      expected: 2
      actual: 1
      operator: 'strictEqual'
      stack: |-
        Object.<anonymous> (file:///home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-chaining.mjs:168:12)
        async ItTest.run (node:internal/test_runner/test:548:9)
        async Promise.all (index 6)
        async Suite.run (node:internal/test_runner/test:798:7)
      ...
    # Subtest: should use the correct `name` for next<HookName>'s function
    not ok 8 - should use the correct `name` for next<HookName>'s function
      ---
      duration_ms: 248.006465
      failureType: 'testCodeFailure'
      error: |-
        The input did not match the regular expression /next<HookName>: nextResolve/. Input:

        '42\nresolve 42\n'

      code: 'ERR_ASSERTION'
      actual: |-
        42
        resolve 42

      operator: 'match'
      stack: |-
        Object.<anonymous> (file:///home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-chaining.mjs:188:12)
        process.processTicksAndRejections (node:internal/process/task_queues:95:5)
        async ItTest.run (node:internal/test_runner/test:548:9)
        async Promise.all (index 7)
        async Suite.run (node:internal/test_runner/test:798:7)
      ...
    # Subtest: should throw for incomplete resolve chain, citing errant loader & hook
    not ok 9 - should throw for incomplete resolve chain, citing errant loader & hook
      ---
      duration_ms: 321.711678
      failureType: 'testCodeFailure'
      error: |-
        The input did not match the regular expression /resolve passthru/. Input:

        ''

      code: 'ERR_ASSERTION'
      actual: ''
      operator: 'match'
      stack: |-
        Object.<anonymous> (file:///home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-chaining.mjs:206:12)
        async ItTest.run (node:internal/test_runner/test:548:9)
        async Promise.all (index 8)
        async Suite.run (node:internal/test_runner/test:798:7)
      ...
    # Subtest: should NOT throw when nested resolve hook signaled a short circuit
    ok 10 - should NOT throw when nested resolve hook signaled a short circuit
      ---
      duration_ms: 333.012634
      ...
    # Subtest: should NOT throw when nested load hook signaled a short circuit
    ok 11 - should NOT throw when nested load hook signaled a short circuit
      ---
      duration_ms: 308.210943
      ...
    # Subtest: should allow loaders to influence subsequent loader resolutions
    not ok 12 - should allow loaders to influence subsequent loader resolutions
      ---
      duration_ms: 327.884519
      failureType: 'testCodeFailure'
      error: |-
        Expected values to be strictly equal:
        + actual - expected

        + 'node:internal/process/esm_loader:19\n' +
        +   "    internalBinding('errors').triggerUncaughtException(\n" +
        +   '                              ^\n' +
        +   '\n' +
        +   "Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'xxx' imported from /home/mzasso/git/nodejs/node/test/fixtures/es-module-loaders/\n" +
        +   '    at new NodeError (node:internal/errors:399:5)\n' +
        +   '    at packageResolve (node:internal/modules/esm/resolve:795:9)\n' +
        +   '    at moduleResolve (node:internal/modules/esm/resolve:844:20)\n' +
        +   '    at DefaultModuleLoader.defaultResolve (node:internal/modules/esm/resolve:1069:11)\n' +
        +   '    at DefaultModuleLoader.resolve (node:internal/modules/esm/loader:298:32)\n' +
        +   '    at DefaultModuleLoader.getModuleJob (node:internal/modules/esm/loader:150:34)\n' +
        +   '    at DefaultModuleLoader.import (node:internal/modules/esm/loader:252:10)\n' +
        +   '    at initializeHooks (node:internal/modules/esm/utils:122:65)\n' +
        +   '    at async setupESMWorker (node:internal/modules/esm/worker:37:13)\n' +
        +   '\n' +
        +   'Node.js v20.0.0-pre\n'
        - ''
      code: 'ERR_ASSERTION'
      expected: ''
      actual: |-
        node:internal/process/esm_loader:19
            internalBinding('errors').triggerUncaughtException(
                                      ^

        Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'xxx' imported from /home/mzasso/git/nodejs/node/test/fixtures/es-module-loaders/
            at new NodeError (node:internal/errors:399:5)
            at packageResolve (node:internal/modules/esm/resolve:795:9)
            at moduleResolve (node:internal/modules/esm/resolve:844:20)
            at DefaultModuleLoader.defaultResolve (node:internal/modules/esm/resolve:1069:11)
            at DefaultModuleLoader.resolve (node:internal/modules/esm/loader:298:32)
            at DefaultModuleLoader.getModuleJob (node:internal/modules/esm/loader:150:34)
            at DefaultModuleLoader.import (node:internal/modules/esm/loader:252:10)
            at initializeHooks (node:internal/modules/esm/utils:122:65)
            at async setupESMWorker (node:internal/modules/esm/worker:37:13)

        Node.js v20.0.0-pre

      operator: 'strictEqual'
      stack: |-
        Object.<anonymous> (file:///home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-chaining.mjs:268:12)
        process.processTicksAndRejections (node:internal/process/task_queues:95:5)
        async ItTest.run (node:internal/test_runner/test:548:9)
        async Promise.all (index 11)
        async Suite.run (node:internal/test_runner/test:798:7)
      ...
    # Subtest: should throw when the resolve chain is broken
    ok 13 - should throw when the resolve chain is broken
      ---
      duration_ms: 338.744039
      ...
    # Subtest: should throw for incomplete load chain, citing errant loader & hook
    not ok 14 - should throw for incomplete load chain, citing errant loader & hook
      ---
      duration_ms: 341.489149
      failureType: 'testCodeFailure'
      error: |-
        The input did not match the regular expression /load passthru/. Input:

        ''

      code: 'ERR_ASSERTION'
      actual: ''
      operator: 'match'
      stack: |-
        Object.<anonymous> (file:///home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-chaining.mjs:309:12)
        process.processTicksAndRejections (node:internal/process/task_queues:95:5)
        async ItTest.run (node:internal/test_runner/test:548:9)
        async Promise.all (index 13)
        async Suite.run (node:internal/test_runner/test:798:7)
      ...
    # Subtest: should throw when the load chain is broken
    ok 15 - should throw when the load chain is broken
      ---
      duration_ms: 256.978548
      ...
    # Subtest: should throw when invalid `specifier` argument passed to `nextResolve`
    ok 16 - should throw when invalid `specifier` argument passed to `nextResolve`
      ---
      duration_ms: 198.392358
      ...
    # Subtest: should throw when resolve hook is invalid
    ok 17 - should throw when resolve hook is invalid
      ---
      duration_ms: 316.932137
      ...
    # Subtest: should throw when invalid `context` argument passed to `nextResolve`
    ok 18 - should throw when invalid `context` argument passed to `nextResolve`
      ---
      duration_ms: 247.714191
      ...
    # Subtest: should throw when load hook is invalid
    ok 19 - should throw when load hook is invalid
      ---
      duration_ms: 268.620979
      ...
    # Subtest: should throw when invalid `url` argument passed to `nextLoad`
    ok 20 - should throw when invalid `url` argument passed to `nextLoad`
      ---
      duration_ms: 229.108731
      ...
    # Subtest: should throw when invalid `url` argument passed to `nextLoad`
    ok 21 - should throw when invalid `url` argument passed to `nextLoad`
      ---
      duration_ms: 206.813966
      ...
    # Subtest: should throw when invalid `context` argument passed to `nextLoad`
    ok 22 - should throw when invalid `context` argument passed to `nextLoad`
      ---
      duration_ms: 276.37821
      ...
    1..22
not ok 1 - ESM: loader chaining
  ---
  duration_ms: 491.227554
  failureType: 'subtestsFailed'
  error: '8 subtests failed'
  code: 'ERR_TEST_FAILURE'
  ...
1..1
# tests 1
# pass 0
# fail 1
# cancelled 0
# skipped 0
# todo 0
# duration_ms 497.194025
Command: out/Release/node /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-chaining.mjs


=== release test-esm-initialization ===
Path: es-module/test-esm-initialization
TAP version 13
# Subtest: ESM: ensure initialization happens only once
    # Subtest: <anonymous>
    not ok 1 - <anonymous>
      ---
      duration_ms: 113.990245
      failureType: 'testCodeFailure'
      error: |-
        Expected values to be strictly equal:

        1 !== 2

      code: 'ERR_ASSERTION'
      expected: 2
      actual: 1
      operator: 'strictEqual'
      stack: |-
        Object.<anonymous> (file:///home/mzasso/git/nodejs/node/test/es-module/test-esm-initialization.mjs:26:12)
        async ItTest.run (node:internal/test_runner/test:548:9)
        async Promise.all (index 0)
        async Suite.run (node:internal/test_runner/test:798:7)
      ...
    1..1
not ok 1 - ESM: ensure initialization happens only once
  ---
  duration_ms: 116.770862
  failureType: 'subtestsFailed'
  error: '1 subtest failed'
  code: 'ERR_TEST_FAILURE'
  ...
1..1
# tests 1
# pass 0
# fail 1
# cancelled 0
# skipped 0
# todo 0
# duration_ms 121.720359
Command: out/Release/node /home/mzasso/git/nodejs/node/test/es-module/test-esm-initialization.mjs


=== release test-code-cache ===
Path: parallel/test-code-cache
node:internal/modules/esm/worker:19
  workerData: { lock, syncCommPort },
                ^

TypeError: Cannot read properties of null (reading 'lock')
    at node:internal/modules/esm/worker:19:17
    at BuiltinModule.compileForInternalLoader (node:internal/bootstrap/loaders:334:7)
    at BuiltinModule.compileForPublicLoader (node:internal/bootstrap/loaders:270:10)
    at loadBuiltinModule (node:internal/modules/helpers:59:9)
    at Module._load (node:internal/modules/cjs/loader:898:20)
    at Module.require (node:internal/modules/cjs/loader:1128:19)
    at require (node:internal/modules/helpers:112:18)
    at Object.<anonymous> (/home/mzasso/git/nodejs/node/test/parallel/test-code-cache.js:19:3)
    at Module._compile (node:internal/modules/cjs/loader:1247:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1301:10)

Node.js v20.0.0-pre
Command: out/Release/node --expose-internals /home/mzasso/git/nodejs/node/test/parallel/test-code-cache.js


[06:27|% 100|+ 3793|-  18]: Done

Failed tests:
out/Release/node --experimental-loader ./test/fixtures/es-module-loaders/assertionless-json-import.mjs /home/mzasso/git/nodejs/node/test/es-module/test-esm-assertionless-json-import.js
out/Release/node --experimental-loader ./test/fixtures/es-module-loaders/example-loader.mjs /home/mzasso/git/nodejs/node/test/es-module/test-esm-example-loader.mjs
out/Release/node --experimental-import-meta-resolve /home/mzasso/git/nodejs/node/test/es-module/test-esm-import-meta-resolve.mjs
out/Release/node /home/mzasso/git/nodejs/node/test/es-module/test-esm-import-flag.mjs
out/Release/node --experimental-loader ./test/fixtures/es-module-loaders/hooks-custom.mjs /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader.mjs
out/Release/node /home/mzasso/git/nodejs/node/test/es-module/test-esm-initialization.mjs
out/Release/node --experimental-loader ./test/fixtures/es-module-loaders/loader-invalid-url.mjs /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-invalid-url.mjs
out/Release/node --loader ./test/fixtures/es-module-loaders/mock-loader.mjs /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-mock.mjs
out/Release/node /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-http-imports.mjs
out/Release/node --experimental-loader ./test/fixtures/es-module-loaders/string-sources.mjs /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-stringify-text.mjs
out/Release/node --experimental-loader ./test/fixtures/es-module-loaders/loader-side-effect.mjs --require ./test/fixtures/es-module-loaders/loader-side-effect-require-preload.js /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-side-effect.mjs
out/Release/node /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-chaining.mjs
out/Release/node --loader ./test/fixtures/es-module-loaders/hook-resolve-type.mjs /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-resolve-type.mjs
out/Release/node /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-thenable.mjs
out/Release/node --experimental-loader ./test/fixtures/es-module-loaders/builtin-named-exports-loader.mjs /home/mzasso/git/nodejs/node/test/es-module/test-esm-named-exports.mjs
out/Release/node /home/mzasso/git/nodejs/node/test/es-module/test-esm-nowarn-exports.mjs
out/Release/node --expose-internals /home/mzasso/git/nodejs/node/test/parallel/test-code-cache.js
out/Release/node /home/mzasso/git/nodejs/node/test/es-module/test-esm-loader-obsolete-hooks.mjs

@targos
Copy link
Author

targos commented Jan 1, 2023

There are still failures related to the console.log calls in test loaders. That's because of the worker.unref(), which prevents some messages from arriving before the process shuts down. We could add an artificial timeout in the test like I did in JakobJingleheimer/worker-atomics#4, or go back to fs.writeSync for this use case.
console.log is not broken in general with this approach.

@GeoffreyBooth GeoffreyBooth force-pushed the feat/esm_off-thread-loaders branch from c8ea77a to 2e60363 Compare January 1, 2023 23:24
@GeoffreyBooth GeoffreyBooth force-pushed the esm-off-thread-unlock-worker branch 2 times, most recently from b07e12a to a04dcae Compare January 2, 2023 00:08
@GeoffreyBooth
Copy link
Collaborator

GeoffreyBooth commented Jan 2, 2023 •
edited
Loading

@targos I rebased JakobJingleheimer/feat/esm_off-thread-loaders off of main now that nodejs#46016 has landed, and then I rebased this accordingly so that you don’t have conflicts. I now see only 17 failures on this branch, compared with 16 on the base. The only new failure is test/es-module/test-esm-initialization.mjs.

However one of the failures on this branch is a timeout, which I’ve grown to dread from the other branch; on the other branch at least, those have always meant that an exception was being thrown inside the worker that never got transferred up to the main thread to be re-thrown there to crash the process. I don’t know if maybe there could be other reasons for a timeout now that the main thread isn’t locked, but ideally we’d fix the timeout (even if the test in question still fails) before merging this in. The timing out test is test/es-module/test-esm-loader-obsolete-hooks.mjs; to cause the timeout directly, rather than inside a spawned child process, run ./node --no-warnings --throw-deprecation --loader ./test/fixtures/es-module-loaders/hooks-obsolete.mjs ./test/fixtures/print-error-message.js.

Also I’m wondering if we can separate the refactor to use postMessage from the unlocking of the main thread. If those can be separate commits or PRs, then I’d think we could benchmark sharedArrayBuffer vs postMessage in isolation. I think postMessage is the more understandable API, but sharedArrayBuffer was originally chosen because the expectation was that it was significantly faster.

@targos targos force-pushed the esm-off-thread-unlock-worker branch from a04dcae to b97e08e Compare January 2, 2023 10:24
@targos
Copy link
Author

targos commented Jan 2, 2023

I think there are the same test failures with e46ff62 alone and with b97e08e (including the timeout you told me about). I'm stopping for now. Maybe I'll investigate the timeout later.

@targos targos force-pushed the esm-off-thread-unlock-worker branch from b97e08e to 1a7cbd4 Compare January 2, 2023 10:29
JakobJingleheimer
JakobJingleheimer reviewed Jan 3, 2023
View reviewed changes
Copy link
Owner

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This looks very promising and far less arcane than Atomics

lib/internal/modules/esm/worker.js
// Send the method response (or exception) to the main thread
try {
TypedArrayPrototypeSet(data, serializedResponse);
syncCommPort.postMessage(response);
Copy link
Owner

@JakobJingleheimer JakobJingleheimer Jan 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure the cause of the huge performance difference between SharedArrayBuffer and postMessage() is because with the SAB, it's a single shared memory space (so the transfer across threads is merely a context switch and nothing is moved/copied); but with postMessage(), when only the first arg is used, the response is copied (which is relatively expensive, on top of consuming double the space).

I think if you instead transfer the response via the 2nd param, we'll see more similar (better) performance with postMessage()

Copy link
Author

@targos targos Jan 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 2nd param of postMessage can only be used with transferable objects. response is not transferable. I still haven't seen any benchmark or perf numbers so I'm not really inclined to talk about it just based on assumptions.

@GeoffreyBooth GeoffreyBooth force-pushed the feat/esm_off-thread-loaders branch 2 times, most recently from dbd039f to 1e6c05e Compare January 7, 2023 21:54
@targos targos force-pushed the esm-off-thread-unlock-worker branch 2 times, most recently from 3f10022 to f224714 Compare January 8, 2023 16:05
targos added 3 commits January 8, 2023 20:31
@targos @GeoffreyBooth
fix doc lint
33b02cb
@targos @GeoffreyBooth
unlock worker thread
fe9111e 
use postMessage from the main thread instead of atomics to signal the worker
@targos @GeoffreyBooth
use syncCommPort to exchange data too
88eac69
@GeoffreyBooth GeoffreyBooth force-pushed the esm-off-thread-unlock-worker branch from f224714 to 88eac69 Compare January 9, 2023 05:02
@GeoffreyBooth GeoffreyBooth merged commit 88eac69 into JakobJingleheimer:feat/esm_off-thread-loaders Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JakobJingleheimer JakobJingleheimer JakobJingleheimer left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@targos @GeoffreyBooth @JakobJingleheimer