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

Document [sub]process.channel.refCounted() and [sub]process.channel.unrefCounted() #53128

Closed
ehmicky opened this issue May 23, 2024 · 2 comments
Labels
feature request Issues that request new features to be added to Node.js. process Issues and PRs related to the process subsystem. stale

Comments

@ehmicky
Copy link

ehmicky commented May 23, 2024

What is the problem this feature will solve?

Background

When using Node.js IPC, if the child process sets a listener on the message or disconnect events, the child process is kept alive as long as those listeners exist. This is good and done automatically by Node.js using newListener and removeListener hooks.

node/lib/child_process.js

Lines 180 to 185 in c137d6e

process.on('newListener', function onNewListener(name) {
if (name === 'message' || name === 'disconnect') control.refCounted();
});
process.on('removeListener', function onRemoveListener(name) {
if (name === 'message' || name === 'disconnect') control.unrefCounted();
});

Users can opt out of this if they want to manually manage this instead, by calling process.channel.ref() and process.channel.unref(). When they do, the automatic reference counting stops happening.

// The methods keeping track of the counter are being used to track the
// listener count on the child process object as well as when writes are
// in progress. Once the user has explicitly requested a certain state, these
// methods become no-ops in order to not interfere with the user's intentions.
refCounted() {
if (++this.#refs === 1 && !this.#refExplicitlySet) {
this.#channel.ref();
}
}
unrefCounted() {
if (--this.#refs === 0 && !this.#refExplicitlySet) {
this.#channel.unref();
this.emit('unref');
}
}
ref() {
this.#refExplicitlySet = true;
this.#channel.ref();
}
unref() {
this.#refExplicitlySet = true;
this.#channel.unref();
}

Problem

I am contributing to a library (Execa) that has a higher-level API built on top of Node.js builtin IPC. For several reasons, I need to proxy IPC messages. To do this, I need to listen to message events. However, that listener should not keep the process alive.

The solution to this problem is currently to call process.channel.unref(). However, this has the following undesirable side effect: if the library's user is also setting up their own message/disconnect event handlers, those will not be automatically reference counted anymore. Which means the child process might accidentally exit too early.

What is the feature you are proposing to solve the problem?

A better solution to this problem is, after setting up message event listeners that should not be ref'd, to call process.channel.unrefCounted() instead (not process.channel.unref()). This has the same desired effect of preventing the child process from being kept alive. But it does not impede on automatic reference counting.

.refCounted() and .unrefCounted() have been added 5 years ago in #30165. They are stable and could be useful for other use cases that the one described above.

My suggestion is: those two methods should be documented. Both for the parent process and the child process. Please note that subprocess.channel.ref(), subprocess.channel.unref(), process.channel.ref() and process.channel.unref() are already documented, so this would effectively document "sibling" methods.

@ehmicky ehmicky added the feature request Issues that request new features to be added to Node.js. label May 23, 2024
@ehmicky ehmicky changed the title Document subprocess.channel.refCounted() and subprocess.channel.unrefCounted() Document [sub]process.channel.refCounted() and [sub]process.channel.unrefCounted() May 23, 2024
@avivkeller avivkeller added process Issues and PRs related to the process subsystem. labels May 24, 2024
Copy link
Contributor

There has been no activity on this feature request for 5 months. To help maintain relevant open issues, please add the never-stale Mark issue so that it is never considered stale label or close this issue if it should be closed. If not, the issue will be automatically closed 6 months after the last non-automated comment.
For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Nov 20, 2024
Copy link
Contributor

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. process Issues and PRs related to the process subsystem. stale
Projects
Archived in project
Development

No branches or pull requests

2 participants