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.

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

cluster: fix edge cases that throw ERR_INTERNAL_ASSERTION #36764

Closed
wants to merge 1 commit into from

Conversation

oyyd
Copy link
Contributor

@oyyd oyyd commented Jan 4, 2021

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:

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 nodejs-github-bot added the cluster Issues and PRs related to the cluster subsystem. label Jan 4, 2021
Copy link
Contributor

@aduh95 aduh95 left a 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?

lib/internal/cluster/child.js Outdated Show resolved Hide resolved
lib/internal/cluster/child.js Outdated Show resolved Hide resolved
lib/internal/cluster/child.js Outdated Show resolved Hide resolved
lib/internal/cluster/child.js Outdated Show resolved Hide resolved
lib/internal/cluster/child.js Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@aduh95
Copy link
Contributor

aduh95 commented Jan 10, 2021

@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@aduh95 aduh95 left a 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

                                                                                                   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
Copy link
Collaborator

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
Copy link
Contributor

aduh95 commented Jan 11, 2021

Landed in 8e3f606

@aduh95 aduh95 closed this Jan 11, 2021
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
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 danielleadams mentioned this pull request Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cluster Issues and PRs related to the cluster subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants