GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — cluster: fix edge cases that throw ERR_INTERNAL_ASSERTION by oyyd · Pull Request #36764 · nodejs/node

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

leeight and osmn00 reacted with thumbs up emoji

All reactions

👍 2 reactions

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

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

lib/internal/cluster/child.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

lib/internal/cluster/child.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

lib/internal/cluster/child.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

lib/internal/cluster/child.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

lib/internal/cluster/child.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

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/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Jan 11, 2021

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aduh95 approved these changes

Jan 11, 2021

View reviewed changes

aduh95 left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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 (***)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

nodejs-github-bot commented Jan 11, 2021

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          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 <[email protected]>

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

oyyd and osmn00 reacted with hooray emoji

All reactions

🎉 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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 <[email protected]>

danielleadams mentioned this pull request

Jan 12, 2021

v15.6.0 proposal #36889

Merged

targos added the backport-blocked-v14.x label

May 25, 2021

Linkgoron mentioned this pull request

Oct 28, 2021

Error [ERR_INTERNAL_ASSERTION]: #40627

Closed

Reviewers

aduh95

aduh95 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.