node icon indicating copy to clipboard operation
node copied to clipboard

http: fix keepAliveTimeout for HTTP server after answering a POST request synchronously

Open not-implemented opened this issue 3 years ago • 2 comments

This is a draft PR for fixing #39137 ... primarily for the test-case for now.

My changes to lib/_http_server.js are just an experiment as a base for discussion - it fixes this issue, but I don't want to fix it that way (and it currently breaks other tests):

  • I don't want a new responseSent flag - I'm sure we can use an existing flag, but I didn't find a suitable one
  • Maybe the resetSocketTimeout() in onParserExecuteCommon() can be even moved to somewhere else? Not when data (body?) arrives, but when the request (header?) arrives
  • It seems resetSocketTimeout() only resets the timeout if keepAliveTimeoutSet, which is set only in resOnFinish ... so maybe the experimental responseSent flag makes keepAliveTimeoutSet useless
  • Resetting responseSent in onParserExecuteCommon() is probably the wrong place anyway ... currently theese two tests are broken:
    • sequential/test-http-server-keep-alive-timeout-slow-client-headers.js
    • sequential/test-http-server-keep-alive-timeout-slow-server.js

not-implemented avatar Jul 07 '21 11:07 not-implemented

@ronag how do you think we can unstuck this PR?

@indutny could you take a look at the top issue https://github.com/nodejs/node/issues/39137? Maybe you have an implementation hint.

mcollina avatar Aug 02 '21 14:08 mcollina

This part of the code is quite tricky. I would have to dig into it.

ronag avatar Aug 02 '21 14:08 ronag