http: reuse socket only when it is drained

[ywave620]

It is not rare for a server to reply a response without reading the whole body,
e.g. a 404 response can be fired once the server sees the url. In the client side,
the response has been completely transferred to the remote peer but there is data
queued to write to the socket(i.e. the socket is not drained).

This PR tries to improve the agent's behavior in this weird case, although weird,
it happens every day in our production environment.

Currently, a socket gets reused by the agent once the entire response is read from
the socket, plus all data of the request are flushed to the socket. In the case
we talked above, such a mechanism might lead to a request to be attached to a non
drained socket, if the request is large(overwhelming the TCP flow control window),
this almost happens every time.

I argue that it is a punishment for a request to be attached to a non drained socket,
because it does not make any progress(no data can be sent to the network), what's
worse, it prevents the request from being assigned to a drained one, which might
occur soon or already in the free pool.

This commit mitigates this problem by defering the time of claiming a socket as free.
Not the time when all data of the current request are flushed to the socket, but the
time when the socket is drained. Note that ensuring every request is assigned to a
drained socket does not add extra latency, compared with the previous code(Writing
to a non drained socket is just effectless).

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/net

[ronag]

I don't quite see what this does. Prefinish and finish is kind of the same thing just slightly different timing.

[ywave620]

prefinish is triggered once the data are flushed to the socket, but it does not mean the socket is drained, finish does. The interval between these two events is tiny in most cases, however, if the remote peer does not extend its TCP flow control window in time or the network is unstable, the interval becomes big.

[ShogunPanda]

I also don't see the point of this PR. Despite of docs and some renaming, I just see that you added an assert.
Is "just" that?

[aduh95]

I also don't see the point of this PR. Despite of docs and some renaming, I just see that you added an assert. Is "just" that?

It's not, the added test is failing on main and passing with this PR.

[ywave620]

I also don't see the point of this PR. Despite of docs and some renaming, I just see that you added an assert. Is "just" that?

I guess it's eaiser to understand what this PR tries to improve through the test I add. Without this change, the second request is assigned to the first socket, which is not drained and will never be drained, hence, it becomes dead forever.

[ShogunPanda]

Thanks, it makes much more sense now.

LGTM!

[mcollina]

lgtm

[nodejs-github-bot]

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

[aduh95]

The test fails on Windows:

node:assert:124
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected "actual" to be strictly unequal to: 63868
    at ClientRequest.<anonymous> (C:\workspace\node-test-binary-windows-js-suites\node\test\parallel\test-http-agent-reuse-drained-socket-only.js:60:12)
    at ClientRequest.<anonymous> (C:\workspace\node-test-binary-windows-js-suites\node\test\common\index.js:438:15)
    at ClientRequest.emit (node:events:513:28)
    at tickOnSocket (node:_http_client:806:7)
    at onSocketNT (node:_http_client:869:5)
    at process.processTicksAndRejections (node:internal/process/task_queues:83:21) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: 63868,
  expected: 63868,
  operator: 'notStrictEqual'
}

Node.js v19.0.0-pre

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[ywave620]

https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu1804_sharedlibs_shared_x64/32944/

Hey guys, these tests failed due to timeout, but I don't think they have anything to do with this PR.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[ywave620]

Hey. All tests have paased. The main branch has commits ahead of my branch, should I intergrate thoses commits into my branch?

I'm not sure this because the main branch changes so fast while PR is a long time process, it's easy for us to run into this situation.

[mcollina]

no need, it can land

[nodejs-github-bot]

Landed in 93e0bf9