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.

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

[20.x backport] worker: add postMessageToThread #57101

Closed
wants to merge 1 commit into from
Closed

[20.x backport] worker: add postMessageToThread #57101

wants to merge 1 commit into from

Conversation

ShogunPanda
Copy link
Contributor

Backport of 66a635c

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v20.x v20.x Issues that can be reproduced on v20.x or PRs targeting the v20.x-staging branch. labels Feb 17, 2025
@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 17, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 17, 2025
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65266/

@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 17, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 17, 2025
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65267/

@ShogunPanda
worker: add postMessageToThread
4e1795f 
PR-URL: nodejs#53682
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
@ShogunPanda ShogunPanda force-pushed the post-message-to-thread-v20.x branch from 2e70ecf to 4e1795f Compare February 17, 2025 14:26
@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 17, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 17, 2025
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65270/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65286/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65287/

@ShogunPanda
Copy link
Contributor Author

@marco-ippolito I have an unrelated error on this PR but all the tests are passing. What shall I do?

@marco-ippolito
Copy link
Member

@marco-ippolito I have an unrelated error on this PR but all the tests are passing. What shall I do?

I only see coverage failing which is fine. Can you paste the error?

@ShogunPanda
Copy link
Contributor Author

https://ci.nodejs.org/job/node-test-binary-windows-js-suites/32736/RUN_SUBSET=0,nodes=win2019-COMPILED_BY-vs2019/testReport/(root)/parallel/test_buffer_tostring_rangeerror/

@marco-ippolito
Copy link
Member

@nodejs/platform-windows we are getting a range error on windows

duration_ms: 259.01
exitcode: 1
severity: fail
stack: |-
  node:assert:377
        throw err;
        ^

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

    Comparison {
  +   name: 'RangeError'
  -   code: 'ERR_STRING_TOO_LONG',
  -   name: 'Error'
    }
      at Object.<anonymous> (C:\workspace\node-test-binary-windows-js-suites\node\test\parallel\test-buffer-tostring-rangeerror.js:21:8)
      at Module._compile (node:internal/modules/cjs/loader:1578:14)
      at Module._extensions..js (node:internal/modules/cjs/loader:1662:10)
      at Module.load (node:internal/modules/cjs/loader:1290:32)
      at Module._load (node:internal/modules/cjs/loader:1106:12)
      at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:164:12)
      at node:internal/main/run_main_module:28:49 {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: RangeError: Array buffer allocation failed
        at new ArrayBuffer (<anonymous>)
        at new Uint8Array (<anonymous>)
        at new FastBuffer (node:internal/buffer:961:5)
        at Function.alloc (node:buffer:397:10)
        at Buffer (node:buffer:275:19)
        at C:\workspace\node-test-binary-windows-js-suites\node\test\parallel\test-buffer-tostring-rangeerror.js:21:21
        at getActual (node:assert:498:5)
        at Function.throws (node:assert:644:24)
        at Object.<anonymous> (C:\workspace\node-test-binary-windows-js-suites\node\test\parallel\test-buffer-tostring-rangeerror.js:21:8)
        at Module._compile (node:internal/modules/cjs/loader:1578:14),
    expected: { code: 'ERR_STRING_TOO_LONG', name: 'Error' },
    operator: 'throws'
  }

@targos
Copy link
Member

targos commented Feb 18, 2025

This looks like an out of memory error. Did it happen multiple times ?

@ShogunPanda
Copy link
Contributor Author

Yes, I retried that several times. But it seems not related to my changes.

@targos
Copy link
Member

targos commented Feb 18, 2025

Unrelated indeed. the daily v20.x build fails with the same test: https://ci.nodejs.org/view/All/job/node-daily-v20.x-staging/472/

@ShogunPanda
Copy link
Contributor Author

So are we good to merge this?

marco-ippolito pushed a commit that referenced this pull request Feb 20, 2025
@ShogunPanda @marco-ippolito
worker: add postMessageToThread
73b5c16 
PR-URL: #53682
Backport-PR-URL: #57101
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
@marco-ippolito
Copy link
Member

Landed in 73b5c16

@ShogunPanda ShogunPanda deleted the post-message-to-thread-v20.x branch February 20, 2025 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marco-ippolito marco-ippolito marco-ippolito approved these changes

Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v20.x v20.x Issues that can be reproduced on v20.x or PRs targeting the v20.x-staging branch.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ShogunPanda @nodejs-github-bot @marco-ippolito @targos