-
Notifications
You must be signed in to change notification settings - Fork 30k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
res.end() from a https.server() handler sends a RST
packet provoking ECONNRESET
#36180
Comments
As this is something specific to the TCP protocol, my proposal is that |
Note that |
Possibly related issue: #27916. |
Yes, that issue looks very similar.
Waiting for the receiving to finish can only be accomplished at the socket level - you can't do it through Node events alone - you need to make your OS send a Also, when directly calling The issue can be visualized by adding |
Yes I understand. My point is that you can't expect a clean closure if you call |
Yes, this is correct, that would be one way to avoid it - when there is such an |
No, the |
@lpinca |
Maybe this could be solved in JS in this case because by the time But this is still a TLS-specific solution, the general case needs solving at the TCP level. |
@lpinca Yes, I confirm
Line 217 in 700612f
node/deps/uv/src/win/handle-inl.h Line 88 in 700612f
Then at the next event loop iteration the socket is simply destroyed: node/deps/uv/src/win/handle-inl.h Line 109 in 700612f
Line 208 in 700612f
There is an no intermediate state - waiting for an |
The situation on Linux is quite different:
node/deps/uv/src/unix/stream.c Line 1280 in 700612f
which in turns calls uv__drain() node/deps/uv/src/unix/stream.c Line 679 in 700612f
However once again it drains only the writing side - it does not wait for an incoming Maybe instead of a |
@lpinca I am afraid there is no solution with @addaleax proposes to avoid adding a new
I think that |
This sounds more like a misunderstanding than a true bug? There's a 'halfClose' flag in nodejs that I think should give the behavior you were looking for. The default is to assume a simultaneous active close, but with that flag toggled, I think you should be able to get the graceful shutdown you describe. I don't see a provision in the TLS standard for halfClose, so I think that is unsupported by that protocol? Thus TLS might not be a great example of "typical" TCP usage, since it permits intentionally causing an RST packet to be sent, and may then expect the other end to ignore certain FIN/RST/shutdown messages at the TLS level (since those aren't encrypted). So that means that this:
is not quite how the TLS standard defines the closure sequence, which you can read for yourself at https://tools.ietf.org/html/rfc5246#section-7.2.1. In summary, to close a socket, the only possible sequence is that active end initiates by sending a close_notify packet. The passive end then should then be "discarding any pending writes". Afterward the TLS connection is now considered closed, and the underlying TCP socket can now be destroyed also. Either uv_close or uv_shutdown may be used, as both will send the same FIN packet at the kernel level. (The FIN packet is also acceptable now to initiating the close sequence as of TLS 1.1, as it was common practice to observe, though may lead to a truncate attack.) Thus, it is reasonable to expect to see FIN/RST exchanged instead of FIN/FIN when using TLS. |
@vtjnash Yes, I think so @mcollina @lpinca Look at this function: node/lib/internal/streams/writable.js Line 648 in 700612f
It triggers the socket shutdown - by calling Now from looking at this code, it seems that you both agree that libuv should do a proper shutdown, all services included. But by looking at the libuv code, I am afraid its authors think that a proper shutdown includes flushing the output side only. In fact, they will call your callback almost immediately. At this moment the So, the million dollar question. Who is responsible for waiting for the incoming data to drain? libuv doesn't do it. |
@vtjnash There is a plan B, where the |
Ah, right, that's for the passive side. I think 'autoClose' is toggle for the active side? node/lib/internal/streams/writable.js Lines 177 to 178 in 700612f
|
@vtjnash, remarkably I have different default values for node/lib/internal/streams/writable.js Line 734 in f47d655
The line before the |
That doesn't appear to directly call node/lib/internal/streams/destroy.js Lines 236 to 237 in 4d54449
Search doesn't seem to show where it would be different by platform either: |
Nothing in streams should be platform dependent. |
@ronag, I will try to isolate once I finish this @vtjnash I have a very elegant solution to the
In this case, there is an incredibly elegant solution that solves the @mcollina @lpinca |
Closing a TCP connection requires 2 FIN/ACK exchanges, so we should wait for both events before destroying a socket Fixes: nodejs#36180
It even solves the |
Ok, just for posteriority:
When you have both, you can destroy the socket (the kernel socket |
@mmomtchev Any updates here? This seems to have stalled and I have a hard time figuring out where/how. |
It's been another 1.5 years with no movement so I'll take the liberty of closing this out. |
What steps will reproduce the bug?
node test/parallel/test-https-truncate.js
- then dump the network trafficThe problem has existed for quite some time, but was masked for most clients and it appeared as a seemingly random problem
How often does it reproduce? Is there a required condition?
node test/parallel/test-https-truncate.js
on Windows is a guaranteed hit#23169 describes an almost guaranteed hit on OSX
Reproduction on Linux is quite difficult and will usually happen only after running in
while /bin/true
loop for some timeWhat is the expected behavior?
res.end()
leads to a proper active closing of the TCP connectionWhat do you see instead?
res.end()
causes the server to directly calluv_close()
which immediately destroys the kernel socket that goes intoTIME_WAIT
state - when the client receives thisFIN
packet he will respond with anACK
and his last data packet - this packet triggers aRST
from the serverAdditional information
There are two ways to properly close a TCP connection:
BYE
message, signals to both ends to simultaneously call the kernelclose()
-uv_close()
for us - thus exchanging twoFIN
/ACK
sequences - today most higher level protocols do provide some form of a bye messageFIN
packet triggered by calling the kernelshutdown()
-uv_shutdown()
in our case. Upon receiving theFIN
packet, the passive end shouldACK
it, then send any remaining data in adata
packet and then proceed to send his ownFIN
. The active end should destroy the connection withclose()
/uv_close()
What currently happens is that when doing
res.end()
from JS this ends innet.Socket.close()
and then goes throughTCPWrap
which does not overloadClose()
and finally inHandleWrap::Close()
Here
uv_close()
is called - this is a direct, unscheduled code-pathWhile the
uv_shutdown()
lies on an indirect code path scheduled by a Dispatch micro-task inLibuvStreamWrap::CreateShutdownWrap()
The result is that the shutdown happens one or two microseconds after the close when in fact a proper close should wait for the shutdown to finish
My opinion is that for TCP connections
uv_close()
should be called only in theuv_shutdown()
callbackHere is the full exchange with all the layers:
And it should be
The text was updated successfully, but these errors were encountered: