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

worker_threads resource limits option not working with cluster module #41066

Open
rafixer opened this issue Dec 3, 2021 · 1 comment
Open
Labels
cluster Issues and PRs related to the cluster subsystem. worker Issues and PRs related to Worker support.

Comments

@rafixer
Copy link

rafixer commented Dec 3, 2021

Version

v17.2.0

Platform

20.6.0 Darwin Kernel Version 20.6.0: Mon Aug 30 06:12:21 PDT 2021; root:xnu-7195.141.6~3/RELEASE_X86_64 x86_64

Subsystem

worker_threads

What steps will reproduce the bug?

index.mjs

import http from 'http';
import cluster from 'cluster';
import path from 'path';
import { Worker } from 'worker_threads';

import v8 from 'v8';

if (cluster.isPrimary) {
  const workersEnv = {
    NODE_OPTIONS: `--max-old-space-size=512`
  };

  cluster.fork(workersEnv);

  const worker = new Worker('./worker.mjs', {
    resourceLimits: {
      maxOldGenerationSizeMb: 200,
    }
  });

  worker.postMessage('main');
} else {
  const worker = new Worker('./worker.mjs', {
    resourceLimits: {
      maxOldGenerationSizeMb: 200,
    }
  });

  worker.postMessage('worker');
}

worker.mjs

import v8 from 'v8';
import { parentPort } from 'worker_threads';

parentPort.on('message', (type) => {
  const { heap_size_limit } = v8.getHeapStatistics();
  console.log(`[${type}] heap_size_limit: ${heap_size_limit}`);
});

output:

[main] heap_size_limit: 260046848
[worker] heap_size_limit: 587202560

How often does it reproduce? Is there a required condition?

Whenever I use worker_threads module with a cluster - worker resourceLimits option is ignored.

What is the expected behavior?

Expected output:

[main] heap_size_limit: 260046848
[worker] heap_size_limit: 260046848

What do you see instead?

Max old space heap size probably was inherited from parent process (cluster) and resource limits option was ignored.

Additional information

This behavior may cause the memory leak on the worker to be handled incorrectly. Worker process will be killed if old heap space reach cluster process limits, not options I've passed.

@Mesteery Mesteery added cluster Issues and PRs related to the cluster subsystem. worker Issues and PRs related to Worker support. labels Dec 3, 2021
@daeyeon
Copy link
Member

daeyeon commented Apr 20, 2022

The primary process above runs with no NODE_OPTIONS, so that the resourceLimits.maxOldGenerationSizeMb seems to work.

A similar case can be observed without cluster module if --max-old-space-size is given in NODE_OPTIONS.

// test.js
const { Worker, isMainThread } = require('worker_threads');
const v8 = require('v8');

if (isMainThread) {
  new Worker(__filename, {
    resourceLimits: { maxOldGenerationSizeMb: 200 }
  });
}
const type = isMainThread ? 'main' : 'work';
console.log(`[${type}] limit: ${v8.getHeapStatistics().heap_size_limit}`);

Output

> node test-worker.js
[main] limit: 4345298944
[work] limit: 260046848

> NODE_OPTIONS=--max-old-space-size=512 node test.js
[main] limit: 587202560
[work] limit: 587202560

When max_old_space_size is concurrently given from both NODE_OPTIONS and the constructor option, the one from NODE_OPTIONS seems to have more priority.

node/deps/v8/src/heap/heap.cc

Lines 5201 to 5207 in e64613d

if (constraints.max_old_generation_size_in_bytes() > 0) {
max_old_generation_size = constraints.max_old_generation_size_in_bytes();
}
if (FLAG_max_old_space_size > 0) {
max_old_generation_size =
static_cast<size_t>(FLAG_max_old_space_size) * MB;
} else if (FLAG_max_heap_size > 0) {

kvakil added a commit to kvakil/node that referenced this issue Jul 26, 2022
This adds a new flag `--thread-max-old-space-size` (name completely
provisional). This has two advantages over the existing
`--max-old-space-size` flag:

1. It allows setting the old space size for the main thread and using
   `resourceLimits` for worker threads. Currently `resourceLimits` will
   be ignored when `--max-old-space-size` is set (see the attached
   issues).
2. It is implemented using V8's public API, rather than relying on V8's
   internal flags whose stability and functionality are not guaranteed.

The downside is that there are now two flags which (in most cases) do
the same thing, so it may cause some confusion. I also think that we
should deprecate `--max-old-space-size`, since the semantics feel pretty
error-prone, but that's a story for another day.

Refs: nodejs#41066
Refs: nodejs#43991
Refs: nodejs#43992
kvakil added a commit to kvakil/node that referenced this issue Jul 26, 2022
This adds a new flag `--thread-max-old-space-size` (name completely
provisional). This has two advantages over the existing
`--max-old-space-size` flag:

1. It allows setting the old space size for the main thread and using
   `resourceLimits` for worker threads. Currently `resourceLimits` will
   be ignored when `--max-old-space-size` is set (see the attached
   issues).
2. It is implemented using V8's public API, rather than relying on V8's
   internal flags whose stability and functionality are not guaranteed.

The downside is that there are now two flags which (in most cases) do
the same thing, so it may cause some confusion. I also think that we
should deprecate `--max-old-space-size`, since the semantics feel pretty
error-prone, but that's a story for another day.

Refs: nodejs#41066
Refs: nodejs#43991
Refs: nodejs#43992
kvakil added a commit to kvakil/node that referenced this issue Jul 26, 2022
This adds a new flag `--thread-max-old-space-size` (name completely
provisional). This has two advantages over the existing
`--max-old-space-size` flag:

1. It allows setting the old space size for the main thread and using
   `resourceLimits` for worker threads. Currently `resourceLimits` will
   be ignored when `--max-old-space-size` is set (see the attached
   issues).
2. It is implemented using V8's public API, rather than relying on V8's
   internal flags whose stability and functionality are not guaranteed.

The downside is that there are now two flags which (in most cases) do
the same thing, so it may cause some confusion. I also think that we
should deprecate `--max-old-space-size`, since the semantics feel pretty
error-prone, but that's a story for another day.

Refs: nodejs#41066
Refs: nodejs#43991
Refs: nodejs#43992
kvakil added a commit to kvakil/node that referenced this issue Jul 26, 2022
This adds a new flag `--thread-max-old-space-size` (name completely
provisional). This has two advantages over the existing
`--max-old-space-size` flag:

1. It allows setting the old space size for the main thread and using
   `resourceLimits` for worker threads. Currently `resourceLimits` will
   be ignored when `--max-old-space-size` is set (see the attached
   issues).
2. It is implemented using V8's public API, rather than relying on V8's
   internal flags whose stability and functionality are not guaranteed.

The downside is that there are now two flags which (in most cases) do
the same thing, so it may cause some confusion. I also think that we
should deprecate `--max-old-space-size`, since the semantics feel pretty
error-prone, but that's a story for another day.

Refs: nodejs#41066
Refs: nodejs#43991
Refs: nodejs#43992
kvakil added a commit to kvakil/node that referenced this issue Jul 26, 2022
This adds a new flag `--thread-max-old-space-size` (name completely
provisional). This has two advantages over the existing
`--max-old-space-size` flag:

1. It allows setting the old space size for the main thread and using
   `resourceLimits` for worker threads. Currently `resourceLimits` will
   be ignored when `--max-old-space-size` is set (see the attached
   issues).
2. It is implemented using V8's public API, rather than relying on V8's
   internal flags whose stability and functionality are not guaranteed.

The downside is that there are now two flags which (in most cases) do
the same thing, so it may cause some confusion. I also think that we
should deprecate `--max-old-space-size`, since the semantics feel pretty
error-prone, but that's a story for another day.

Refs: nodejs#41066
Refs: nodejs#43991
Refs: nodejs#43992
kvakil added a commit to kvakil/node that referenced this issue Jul 26, 2022
This adds a new flag `--thread-max-old-space-size` (name completely
provisional). This has two advantages over the existing
`--max-old-space-size` flag:

1. It allows setting the old space size for the main thread and using
   `resourceLimits` for worker threads. Currently `resourceLimits` will
   be ignored when `--max-old-space-size` is set (see the attached
   issues).
2. It is implemented using V8's public API, rather than relying on V8's
   internal flags whose stability and functionality are not guaranteed.

The downside is that there are now two flags which (in most cases) do
the same thing, so it may cause some confusion. I also think that we
should deprecate `--max-old-space-size`, since the semantics feel pretty
error-prone, but that's a story for another day.

Refs: nodejs#41066
Refs: nodejs#43991
Refs: nodejs#43992
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. worker Issues and PRs related to Worker support.
Projects
None yet
Development

No branches or pull requests

3 participants