FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

cluster: fix edge cases that throw ERR_INTERNAL_ASSERTION by oyyd · Pull Request #36764 · nodejs/node · GitHub

/ node Public

cluster: fix edge cases that throw ERR_INTERNAL_ASSERTION - #36764

Closed
oyyd wants to merge 1 commit into
nodejs:masterfrom
oyyd:cluster-child-index
ConversationCommits (1)ChecksFiles changed
Closed

cluster: fix edge cases that throw ERR_INTERNAL_ASSERTION#36764
oyyd wants to merge 1 commit into
nodejs:masterfrom
oyyd:cluster-child-index

Conversation

oyyd commented Jan 4, 2021

Copy link
Copy Markdown
Contributor

Some cases use both cluster and net/cluser will throw
ERR_INTERNAL_ASSERTION when listen/bind to the port of 0, like below:

const cluster = require('cluster')
const dgram = require('dgram')

const kPort = 0;

function child() {
  for (let i = 0; i < 2; i += 1) {
    const socket = new dgram.Socket('udp4')
    socket.bind(kPort)
    setTimeout(() => {
      socket.close()
      const socket2 = new dgram.Socket('udp4')
      socket2.bind(kPort);
    }, 100)
  }
}

if (cluster.isMaster) {
  cluster.fork(__filename)
} else {
  child()
}

would throw:

internal/assert.js:14
    throw new ERR_INTERNAL_ASSERTION(message);
    ^

Error [ERR_INTERNAL_ASSERTION]: This is caused by either a bug in Node.js or incorrect usage of Node.js internals.
Please open an issue with this stack trace at https://github.com/nodejs/node/issues

    at assert (internal/assert.js:14:11)
    at SharedHandle.add (internal/cluster/shared_handle.js:28:3)
    at queryServer (internal/cluster/master.js:309:10)
    at Worker.onmessage (internal/cluster/master.js:249:5)
    at ChildProcess.onInternalMessage (internal/cluster/utils.js:47:8)
    at ChildProcess.emit (events.js:326:22)
    at emit (internal/child_process.js:906:12)
    at processTicksAndRejections (internal/process/task_queues.js:81:21) {
  code: 'ERR_INTERNAL_ASSERTION'
}

After some investigation, I believe it's because we remove the indexesKey when close servers while it might reference more than one index:

node/lib/internal/cluster/child.js

Line 127 in 8b8620d

indexes.delete(indexesKey);

This PR maitains a separate map of the index to fix the issue.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

nodejs-github-bot added the cluster Issues and PRs related to the cluster subsystem. label Jan 4, 2021
aduh95 reviewed Jan 4, 2021
View reviewed changes

aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Shouldn't we use a SafeSet instead of a SafeMap?

Comment thread lib/internal/cluster/child.js Outdated
Comment thread lib/internal/cluster/child.js Outdated
Comment thread lib/internal/cluster/child.js Outdated
Comment thread lib/internal/cluster/child.js Outdated
Comment thread lib/internal/cluster/child.js Outdated
oyyd force-pushed the cluster-child-index branch from 6945f24 to 89c8d53 Compare January 10, 2021 11:59

nodejs-github-bot commented Jan 10, 2021

Copy link
Copy Markdown
Collaborator

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

aduh95 commented Jan 10, 2021

Copy link
Copy Markdown
Contributor

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/884/

nodejs-github-bot commented Jan 11, 2021

Copy link
Copy Markdown
Collaborator

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

aduh95 approved these changes Jan 11, 2021
View reviewed changes

aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Benchmark CI didn't show any perf regressions or improvements

Details 
                                                                                                   confidence improvement accuracy (*)    (**)   (***)
 cluster/echo.js n=100000 serialization='advanced' sendsPerBroadcast=10 payload='object' workers=1                -2.39 %       ±6.11%  ±8.13% ±10.58%
 cluster/echo.js n=100000 serialization='advanced' sendsPerBroadcast=10 payload='string' workers=1                -1.81 %       ±7.25%  ±9.65% ±12.57%
 cluster/echo.js n=100000 serialization='advanced' sendsPerBroadcast=1 payload='object' workers=1                 -6.29 %      ±16.92% ±22.52% ±29.32%
 cluster/echo.js n=100000 serialization='advanced' sendsPerBroadcast=1 payload='string' workers=1                  0.04 %       ±7.12%  ±9.48% ±12.34%
 cluster/echo.js n=100000 serialization='json' sendsPerBroadcast=10 payload='object' workers=1                    -8.05 %      ±11.64% ±15.52% ±20.29%
 cluster/echo.js n=100000 serialization='json' sendsPerBroadcast=10 payload='string' workers=1                    10.75 %      ±11.97% ±15.99% ±20.92%
 cluster/echo.js n=100000 serialization='json' sendsPerBroadcast=1 payload='object' workers=1                     -9.79 %      ±11.02% ±14.67% ±19.09%
 cluster/echo.js n=100000 serialization='json' sendsPerBroadcast=1 payload='string' workers=1                      4.68 %      ±11.14% ±14.82% ±19.29%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case there are 8 comparisons, you can thus
expect the following amount of false-positive results:
  0.40 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.08 false positives, when considering a   1% risk acceptance (**, ***),
  0.01 false positives, when considering a 0.1% risk acceptance (***)

nodejs-github-bot commented Jan 11, 2021

Copy link
Copy Markdown
Collaborator

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

cluster: fix edge cases that throw ERR_INTERNAL_ASSERTION
8e3f606 
Some cases use both `cluster` and `net`/`cluser` will throw
ERR_INTERNAL_ASSERTION when `listen`/`bind` to the port of `0`. This
PR maitains a separate map of the index to fix the issue. See the new
tests added for the detail cases.

PR-URL: nodejs#36764
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
aduh95 force-pushed the cluster-child-index branch from 89c8d53 to 8e3f606 Compare January 11, 2021 10:27

aduh95 commented Jan 11, 2021

Copy link
Copy Markdown
Contributor

Landed in 8e3f606

aduh95 closed this Jan 11, 2021
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
cluster: fix edge cases that throw ERR_INTERNAL_ASSERTION
54bd4ab 
Some cases use both `cluster` and `net`/`cluser` will throw
ERR_INTERNAL_ASSERTION when `listen`/`bind` to the port of `0`. This
PR maitains a separate map of the index to fix the issue. See the new
tests added for the detail cases.

PR-URL: #36764
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams mentioned this pull request Jan 12, 2021
Merged
Linkgoron mentioned this pull request Oct 28, 2021
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

aduh95 aduh95 approved these changes

Assignees

No one assigned

Labels

cluster Issues and PRs related to the cluster subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL