investigate flaky test/parallel/test-http-many-ended-pipelines.js

[Trott]

• Test: test/parallel/test-http-many-ended-pipelines.js
• Platform: macOS 10.14, macOS 10.15
• Console Output:

HTTP/1.1 200 OK
Date: Tue, 09 Feb 2021 08:14:57 GMT
Connection: keep-alive
Keep-Alive: timeout=5
Content-Length: 2

ok
node:events:355
      throw er; // Unhandled 'error' event
      ^

Error: read ECONNRESET
    at TCP.onStreamRead (node:internal/stream_base_commons:211:20)
Emitted 'error' event on Socket instance at:
    at emitErrorNT (node:internal/streams/destroy:192:8)
    at emitErrorCloseNT (node:internal/streams/destroy:157:3)
    at processTicksAndRejections (node:internal/process/task_queues:81:21) {
  errno: -54,
  code: 'ECONNRESET',
  syscall: 'read'
}

• Build Links: https://ci.nodejs.org/job/node-test-commit-osx/38908/nodes=osx1014/testReport/junit/(root)/test/parallel_test_http_many_ended_pipelines/

[Trott]

I'm able to replicate this locally with this command:

tools/test.py -j96 --repeat=1920 test/parallel/test-http-many-ended-pipelines.js

If I remove the -j96, I am unable to replicate it. So that suggests some sort of resource constraint (memory? CPU? network? file descriptors? something) makes the issue more likely to happen.

[Trott]

I know there's been some refactoring of streams and stream-like http/net events, and even some small breaking changes I believe.

@ronag @lpinca Any idea if the bug here is in the test code or if it's probably indicative of a bug in Node.js core?

[lpinca]

It seems to me that the test is flaky by design. The socket might be destroyed server-side while the client is still writing to it.

[ronag]

I think the test is missing a 'error' handler. An ´ECONNRESET` looks like a expected/normal thing here.

[lpinca]

The issue can be exacerbated by increasing numRequests. Setting it to 2000 makes the test always fail on my machine.

Also, there seems to be an unwanted MaxListenersExceededWarning caused by

node/lib/_http_incoming.js

Lines 180 to 183 in 33d3a2e

	const cleanup = finished(this.socket, (e) => {
	cleanup();
	onError(this, e || err, cb);
	});

. I think the condition here

node/lib/_http_incoming.js

Line 178 in 33d3a2e

if (this.socket && this.aborted) {

should prevent finished() from being called again with the same socket argument.

[lpinca]

@ronag, the second issue (the unwanted warnings) seems to be caused by 2da3611. I'm not sure if it makes sense to revert it.

[ronag]

@ronag, the second issue (the unwanted warnings) seems to be caused by 2da3611. I'm not sure if it makes sense to revert it.

I don't think we should revert. I'll see if I can find a better solution.