nodejs / nodejs/node

net: setKeepAlive() ignores the error returned by the handle

Open
#65,529 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
JavaScript
Stars
122k
Forks
37.3k
Avg merge
4d 2h
Merged PRs (30d)
283

Description

Version

v27.0.0-pre (main)

Platform

Darwin 25.4.0 arm64

Subsystem

net

What steps will reproduce the bug?
const net = require('net');
const server = net.createServer().listen(0, () => {
  const socket = net.connect(server.address().port, () => {
    // Looks like it succeeded
    const returned = socket.setKeepAlive(true, -5000);
    console.log('returns socket:', returned === socket);

    // The handle actually returned EINVAL
    const err = socket._handle.setKeepAlive(true, -5, 1, 10);
    console.log('handle returns:', err);

    socket.destroy();
    server.close();
  });
});

Output:

returns socket: true
handle returns: -22
How often does it reproduce? Is there a required condition?

Every time, for any value the platform rejects.

What is the expected behavior? Why is that the expected behavior?

socket.setKeepAlive() should not report success when the underlying
operation failed. setTypeOfService(), right next to it in the same file,
already does this:

const err = this._handle.setTypeOfService(tos);
if (err && !isWindows) {
  throw new ErrnoException(err, 'setTypeOfService');
}
What do you see instead?

setKeepAlive() discards the return value of the handle call:

this._handle.setKeepAlive(enable, initialDelay, interval, count);

TCPWrap::SetKeepAlive does propagate the error to JS
(args.GetReturnValue().Set(err) in src/tcp_wrap.cc), and
uv_tcp_keepalive_ex() returns UV_EINVAL for a negative delay, so the
information is available. It is simply dropped.

The caller has no way to tell: the return value is the socket either way,
and no error is thrown or emitted.

Additional information

Found while working on #57712 / #65528, which adds a warning for delays
below 1000 ms that are truncated to 0. That change is about values which
are silently reduced; this one is about an error which is silently
discarded, so it seemed better to report it separately.

I have not checked how a value above the platform limit behaves on other
systems. On macOS setKeepAlive(true, 40000000) (40000 s) is accepted by
the handle, so any range check would need per-platform investigation first.

I am happy to open a PR if the direction is agreed. Whether a failure
should throw, like setTypeOfService(), or only emit a warning is a
decision I would rather leave to the team, since throwing would be a
breaking change.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the socket.setKeepAlive() path and TCPWrap::SetKeepAlive in src/tcp_wrap.cc, then reproduce the negative-delay case from the issue. Compare the handling with setTypeOfService() and investigate the platform behavior noted for macOS and other systems. Done means underlying failures are no longer silently discarded, using the team-approved error behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp, javascript, nodejs
Domain
networking
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.