http: use listenerCount when adding noop event

[ShogunPanda]

Fixes #43548.
See discussion in #46523.

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/net

[lpinca]

See discussion in #46523 (comment)

[ShogunPanda]

@lpinca

I see your point about not being a bug in Node.js. I still think we should prevent the fix anyway. So, can please others gave a final vote on this?

Other than that: do you have anything else you want me to change in this PR that I've lost in the discussion in the other issue?

[nodejs-github-bot]

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

[lpinca]

I still think we should prevent the fix anyway.

The fix here actually hides the user error of keeping the socket open. If others think this is a good idea then I will dismiss my request for changes.

The events leak warning might be unhelpful but at least it tells the user that there is something wrong.

[mcollina]

lgtm

[ShogunPanda]

@nodejs/http Can somebody else upvote or downvote this so we can reach an agreement?

[mcollina]

There is a lot of legacy code out there that emits 'error' on streams manually. I think this is needed.

[lpinca]

There is a lot of legacy code out there that emits 'error' on streams manually. I think this is needed.

I disagree. Note that there is no crash, only a warning and this silences it. Instead of informing the user that there is something wrong in their code, this just ignores it. In my opinion it is not an improvement, it only promotes invalid usage. Just look at the second test added here.

[mcollina]

The problem is that the user must learn how to fix it. Most of those behaviors are codified in old libraries that are deep in the dependency chain, and it's close to impossible for most users to understand what's going on. My 2 cents is that we must convince them to switch from Node v0.8-era dependencies, as I don't expect those modules to be fixed.

If we want to emit a warning for this case, it should be actionable, e.g. "an error event has already been emitted on the request. Please use the destroy method instead".

[lpinca]

If we want to emit a warning for this case, it should be actionable, e.g. "an error event has already been emitted on the request. Please use the destroy method instead".

Yes that would be perfect. Currently, a bad warning is better than no warning.

[ShogunPanda]

So I leave the "memory leak" prevention and I add an additional warning so the user is beware of the problematic handling.
Are we good on this?

[mcollina]

Yes, exactly!

[lpinca]

I have no objection with that. Note that there might be other cases where the 'error' event can be emitted multiple times. I think this new warning should only handle the case where the 'clientError' event is used incorrectly.

[ShogunPanda]

@mcollina @lpinca What about now? Are we good to go?

[nodejs-github-bot]

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

[dougwilson]

I have a question. The only code needed to make the original warning appear is the following:

const fs = require('fs');
const http = require('http');

const app = (req, res) => {
  res.end(JSON.stringify({
    message: 'Hello World!'
  }));
};

const httpServer = http.createServer(app);

httpServer.listen(80, () => {
    console.log('HTTP Server running on port 80');
});

Refer to original #43548 for full reproduction steps.

Does this mean that in order to make a basic Node.js http server will also require added a clientError listener to close the socket?

[lpinca]

@dougwilson

Are you able to reproduce the issue with that test case?

Does this mean that in order to make a basic Node.js http server will also require added a clientError listener to close the socket?

No, because that is what Node.js does by default if the user does not add a listener for the 'clientError' event.

[dougwilson]

Are you able to reproduce the issue with that test case?

Yes... I am using Jmeter from another machine. I will try it here on some various Node.js versions in a moment.

No, because that is what Node.js does by default if the user does not add a listener for the 'clientError' event.

Ok, then I am confused here. This PR says it is suppsoed to fix #43548 but that issue is pretty basic and doesnt add clientError listener. Is this PR to fix a different issue? I am only trying to follow the fix for #43548 so if this is not for that, please forgive me for injecting.

[lpinca]

I think the whole discussion originates from #43587 where a test case was added in the form of an invalid 'clientError' listener. If the issue can be reproduced with the code in #46769 (comment), then it is a different story and my objections here fall.

[dougwilson]

I am trying to whittle down a simple ideally self-contained example to reproduce the conditions Jmeter is making with the load test.

So far I at least seem to have a small example that shows what the noop listener is for, at least, if that helps in any way. Removing it causes this test to crash with unhandled exception from the server. The change in this PR doesn't break this test, which is good. It may be useful to add this to the Node.js test suite too, if it is not expected for users to add an error event handler to a basic server (not sure if that is a correct assumption -- but it seems that is what the purpose of the noop adding is). I tried this PR and it works just fine here too because it adds the noop, and for this test case socketOnError is only called the one time for the socket.

const http = require('http')

const server = http.createServer((req, res) => {
  res.end(JSON.stringify({ hello: 'world' }))
})

server.listen(3000, '127.0.0.1')

const net = require('net')

server.on('listening', () => {
  const socket = net.connect(3000, '127.0.0.1')
  socket.on('connect', () => {
    socket.write('POST /s HTTP/1.1\r\nHost: localhost\r\nTransfer-Encoding: chunked\r\n\r\n')
    socket.resume()
    setTimeout(() => {
      socket.end('z')
    }, 1000)
  })
  socket.on('close', () => server.close())
})

[lpinca]

@dougwilson yes but that example does not cause the "leak" warning, does it?

[dougwilson]

@dougwilson yes but that example does not cause the "leak" warning, does it?

No. I am trying to whittle down a simple ideally self-contained example to reproduce the conditions Jmeter is making with the load test.

[ShogunPanda]

@lpinca I think the issue @dougwilson can eventually be handle as separate issue. Do you still want changes on this or are we good to go?

[lpinca]

Yes, let's wait a little more for @dougwilson's feedback.

[ShogunPanda]

@lpinca No more input received. Shall we wait more or can we land this?

[lpinca]

Wait a few more days, maybe @dougwilson is busy. It is not something that needs an immediate fix and I would prefer not to revert it again.

[nodejs-github-bot]

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

[mcollina]

lgtm

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 0d3faae