short body race condition in http parsing

[chad3814]

Version

v16.14.0

Platform

Linux chad1 5.4.0-99-generic #112~18.04.1-Ubuntu SMP Thu Feb 3 14:09:57 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

http net.Socket streams

What steps will reproduce the bug?

In this gist: https://gist.github.com/chad3814/b1f927c5cabae86fb2733f1ab37d6bfd
there are two files, listener.mjs and putter.mjs. If you run listener.mjs and then in another shell run putter.mjs you'll see that it saves an empty, zero byte file. Then if you remark the await sleep(10); on line 12 on listener.mjs and run them again, you'll see that it writes out the file on the first pass through the loop, but on the next pass an ECONNRESET is thrown.

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

Happens every time as discussed above

What is the expected behavior?

What it looks like to me is when the body of an http message is short, the header and body don't break the high water mark, so it's all read in at once and saved in the buffer list. The problem is triggered by the remote end not waiting for a response from the server and just closing the connection (which is allowed afaik by the http spec). I can understand an ECONNRESET being thrown, but we should be able to read the data from the body. Or maybe the error should be thrown if we/when we write to the response object? Either way, we need to reliably read the data using node 16 like we did with node 14.

What do you see instead?

I see this problem in our production code because ffmpeg does the same sort of http call as in putter.mjs, where it pushes all the data and then closes the connection without waiting for a response. This all worked in node 14 using .pipe()s but with node 16 and pipes we're seeing the two issues above at different times.

Additional information

No response

[chad3814]

@ronag any idea?

[Trott]

@nodejs/http

[mcollina]

@chad3814 you mentioned you were running a .pipe() call in Node v14, however I do not see any pipe() call in your example but async iterators. Can you please clarify where is the regression? What code was working in which version?

[chad3814]

@chad3814 you mentioned you were running a .pipe() call in Node v14, however I do not see any pipe() call in your example but async iterators. Can you please clarify where is the regression? What code was working in which version?

sorry, the pipe version fails inconsistently and is slightly more complex, so I shared the async iterator version with fails every time and is less complex. I'll make a second listener.mjs that uses .pipe() to the same effect.

[chad3814]

okay @mcollina I added listener-pipe.mjs to the gist https://gist.github.com/chad3814/b1f927c5cabae86fb2733f1ab37d6bfd and it also fails in the way described above. With the sleep remarked then the listener throws an ECONNRESET error. With a 10ms sleep, everything works usually.

[chad3814]

okay, forgot about the regress question.
node 14.18.3 the pipe version works every time with or without the sleep.
node 16.14.0 the pipe version works sometimes with the sleep but fails with ECONNRESET without the sleep
node 16.14.0 the async iterator version throws ECONNRESET with the sleep, and doesn't write anything without the sleep

[ronag]

This is tricky... the problem here is that the req/res are coupled so that if one fails the other one fails immediately. In this case the response fails before the request body has been read.

I can't think of a good way to fix this. Calling read() before destroying the request is one option but I think that will only work for the case where .pipe is used.

[ckirmse]

@ronag I don't know the node internals, but it seems to me that in node <= 14 the ECONNRESET on the req was being treated as end-of-stream (and not emitted as an error), which also seems like the right behavior to me since ECONNRESET just indicates the socket was closed on the remote side. Would it be possible to treat it that way in the new (node >= 15) stream code and would that be the proper behavior in your opinion?

[imaustink]

Is there a workaround other than adding a delay? This is blocking us from moving several services to Node 16.

[chad3814]

Is there a workaround other than adding a delay? This is blocking us from moving several services to Node 16.

We stopped trying to move to node 16 because of this. Even if you add a delay, you will lose the short data, so that’s a non-starter.

[ShogunPanda]

I cannot reproduce this anymore in Node 16.15.0 (I'm on MacOS)
I wonder if my default disabling of the Nagle's algorithm in #41310 fixed it somehow.
Can you please check it again and let me know if it still an issue?

[ShogunPanda]

@chad3814 Any update on this?

[ShogunPanda]

No updated after two months. Closing this.