Calling .end on an already finished http response leaves the socket instance corked

[mitsos1os]

• Version: v14.15.3
• Platform: Linux pop-os 5.8.0-7630-generic #32~1606339263~20.10~61c3910-Ubuntu SMP Thu Nov 26 00:10:35 UTC x86_64 x86_64 x86_64 GNU/Linux
• Subsystem: lib/_http_outgoing.js

What steps will reproduce the bug?

I am facing a situation in Nodejs v14.15.3, (it also affects the latest version), which is quite an edge-case, since it requires a specific set of circumstances to be present, but I believe is a bug.

The main problem is that calling .end on an HTTP OutgoingMessage that is already marked as finished (from a previous call of .end), leaves the underlying socket instance in a corked state.

This would not be a problem in case of every request being handled by an explicitly different socket instance.
However, in case of multiple simultaneous requests there is some socket reuse. This way, the response handling mechanism is given a socket instance that is already corked from the beginning. Following operations of cork -> uncork on the socket are performed an equal number of times, so they leave it in the initial corked state, leading the data in buffer never to be flushed.
Also, if sending small responses where .end is directly called with the data, will NOT reproduce the issue, since .end successfully flushes the data, no matter how many levels of cork exist: L827. So the bug appears on requests where socket has a lot of data to send and reaches a point that needs to drain in order to move forward, which will never happen, because the socket remains in a corked state, so the response hangs!

To sum up, steps to reproduce:

1. Multiple simultaneous requests, so that we have socket re-use
2. Call .end on an http server response
3. write something on the socket big enough, (larger than highWatermark) to require a drain (like a file stream from fs.createReadStream)

How often does it reproduce? Is there a required condition?

It will happen every time if the above conditions are met, as long as an already existing socket instance is picked up for re-use.
On my setup (where the actual issue was observed) a local dev environment that issues 30-40 requests (for API calls and static files), always reproduces the problem.

What is the expected behavior?

The http response should complete successfully no matter if socket re-use is taking place in the background. It should be abstract to the user.

What do you see instead?

What happens, is that the response hangs and never completes since the data always remain in the buffer and never flushed for the system to handle. In a browser you will see the loading spinning wheel in the adress bar spin forever.

Additional information

I have backtracked the issue and it was introduced around version v13.8.0 in commit c776a37 which moved the check for finished socket further below, (here), allowing the socket to first get corked and then exit if already finished.

If you agree this is an unwanted behavior I would be happy to open a Pull Request to resolve this!

[ronag]

#36620

[ronag]

Closing this as a duplicate (#36620).

[mitsos1os]

Oh shit..... I had pindowned the issue on Wednesday 4 days ago..., and thought I should rest a couple of days for Christmas and report it later..... Can't believe this was actually reported at exactly the same time 😑
Sorry for that!

[ronag]

Sorry for that!

No worries! Thanks for the additional info.

[kachkaev]

Wow, I can’t believe that both me and @mitsos1os got caught by the same bug simultaneously, especially given that it was around for months since Node 14.0.0 😅 Thanks for the extra info indeed!

Can we call our two bug reports a Christmas bingo? Even if not, this is definitely my ‘bug of the year’ and I’m popping a Champaign cork() to @ronag’s #36633 🍾 😁

[mitsos1os]

@kachkaev exactly my point!!! I was... "how come nobody saw this for more than year???" and then I decided that it waited for a year, so it could also wait a couple days for me to enjoy my christmas eve... hahahaha that was really crazy....