test: do not race server.close with timeout - #44039
Conversation
Sorry, something went wrong.
server.close() should not close the connection as the request is in progress, it should only prevent new connections from being accepted, no? |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
| @@ -9,6 +9,6 @@ server.listen(common.mustCall(() => { | |||
| http.get({ port: server.address().port }, common.mustCall((res) => { | |||
| res.on('timeout', common.mustCall(() => req.destroy())); | |||
There was a problem hiding this comment.
| 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.
Sorry, something went wrong.
There was a problem hiding this comment.
Failure will require a timeout and kill from the runner in this case?
Sorry, something went wrong.
There was a problem hiding this comment.
Failure will require a timeout and kill from the runner in this case?
Yes.
Sorry, something went wrong.
| res.on('timeout', common.mustCall(() => req.destroy())); | ||
| res.setTimeout(1); | ||
| server.close(); | ||
| setTimeout(common.mustCall(() => server.close()), 2); |
There was a problem hiding this comment.
| setTimeout(common.mustCall(() => server.close()), 2); |
Sorry, something went wrong.
|
Anyway I also think that Server.prototype.closeIdleConnections() incorrectly closes the connection. The client connection is waiting for a response. |
Sorry, something went wrong.
|
About that test, I see timeouts too narrow. I wonder if events get scheduled in the wrong way in the same tick. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
@lpinca, there is also libuv/libuv#3686 which is the real reason I am fixing this |
Sorry, something went wrong.
|
@lpinca @ShogunPanda |
Sorry, something went wrong.
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.