test: do not race server.close with timeout

[mmomtchev]

Refs: #43680

This fails when server.close() manages to close the connection before the timeout has fired.
Here is the original issue: #33734
It is not about the behaviour when racing server.close() with the timeout - it is about the timeout not firing at all. I am removing the race condition as I don't think there should be any guarantees on which callback should fire first.

[lpinca]

This fails when server.close() manages to close the connection before the timeout has fired

server.close() should not close the connection as the request is in progress, it should only prevent new connections from being accepted, no?

[mmomtchev]

This fails when server.close() manages to close the connection before the timeout has fired

server.close() should not close the connection as the request is in progress, it should only prevent new connections from being accepted, no?

This connection appears to be an idle connection to server.close() and so it is closed by Server.prototype.closeIdleConnections. I am not sure if this is expected or a bug.

[lpinca]

Ok it makes sense. It looks like Server.prototype.close() behavior was changed in #43522. I think there should be at least an option to preserve the old behavior. cc: @ShogunPanda

[lpinca]

Suggested change

	res.on('timeout', common.mustCall(() => req.destroy()));
	res.on('timeout', common.mustCall(() => {
	req.destroy();
	server.close();
	}));

The original issue was that the listener of the 'timeout' event was not called if added on the response object so this should be ok. It does not invalidate the test and only one timer is used.

[mmomtchev]

Failure will require a timeout and kill from the runner in this case?

[lpinca]

Failure will require a timeout and kill from the runner in this case?

Yes.

[lpinca]

Suggested change

setTimeout(common.mustCall(() => server.close()), 2);

[lpinca]

Anyway I also think that Server.prototype.closeIdleConnections() incorrectly closes the connection. The client connection is waiting for a response.

[ShogunPanda]

@lpinca In #43890 I fixed that behavior. The method will not close connections which are still awaiting a response.

[ShogunPanda]

About that test, I see timeouts too narrow. I wonder if events get scheduled in the wrong way in the same tick.

[lpinca]

FWIW no failures have been registered for this test in last 5 days, so I guess #43890 fixed the underlying issue. I think it is better to keep the test as is if it no longer fails.

[mmomtchev]

@lpinca, there is also libuv/libuv#3686 which is the real reason I am fixing this

[mmomtchev]

@lpinca @ShogunPanda
In fact #43890 fixed both problems - the flaky unit test and the dependency on the uv event loop behaviour.
I am closing this PR, @danielleadams, you should probably unmark this unit test as being flaky