stream: remove kludge masking RST on Windows #35946

mmomtchev · 2020-11-03T15:59:06Z

Remove the kludge that masks the TCP RST on
Windows on test-https-truncate
That RST is very real, originates from the server
and should be investigated

Fixes: #35904

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Remove the kludge that masks the TCP RST on Windows on test-https-truncate That RST is very real, originates from the server and should be investigated Fixes: nodejs#35904

ronag

This is here because libuv for some reason call read after eof on windows.

mmomtchev · 2020-11-03T16:29:16Z

It calls it because it receives a RST from the server. The behavior of the client is correct.
The real question is what does the TLS/HTTPS/libuv server code do to make Windows send a RST.
On a POSIX system the two most common reasons would be

closing a socket that has data waiting to be read
setting an (aggressive) lingering option
On a Windows it could be something else too - Windows used to be infamous for its RSTs (it seems it is much better now that some years ago)
But for me, this a server-side problem - the client simply reports that it received a RST - which it does receive

mmomtchev · 2020-11-03T17:24:37Z

OSX does it too
Hopefully it will be the same issue on both platforms

mmomtchev · 2020-11-03T18:03:57Z

It is related to the handling of the res.end() in the legacy http code - it effectively closes the socket while incoming data is still waiting in it. I am digging further, but this is definitely an http server problem - nothing to do with stream.

On Windows and OSX, the HTTP server will often make the OS send a TCP RST packet by shutting down the socket before all incoming data has been read. Normally, libuv should take care of this (?), but it does not on Windows and OSX Refs: nodejs#35904

On Windows, do not skip delaying the closesocket when the socket is not shared with another process - I don't see any reason which will allow this optimization Refs: nodejs#35946 Refs: libuv/libuv#3034

mmomtchev · 2020-11-04T14:18:23Z

@ronag This is the real fix, however this needs to be merged in libuv
I tried working around the issue in Node, but there are lots of special cases to handle and this does not belong in Node anyway

mmomtchev · 2020-11-04T14:34:44Z

libuv/libuv#3035

The comment here seems mixed up between send and recv buffers. The default behavior on calling `closesocket` is already to do a graceful shutdown (see https://docs.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-closesocket with default l_onoff=zero) if it is the last open handle. The expect behavior if there are pending reads in flight is to send an RST packet, notifying the client that the server connection was destroyed before acknowledging the EOF. Refs: libuv#3035 Refs: nodejs/node#35946 Refs: nodejs/node#35904 Fixes: libuv#3034 PR-URL:

This is an attempt to fix some resource management issues on Windows. Win32 sockets has a bug where it sends an RST packet if there is an outstanding overlapped WSARecv call. We can avoid that by being certain to explicitly cancel our read request first. This also removes some conditional cleanup code, since we might as well clean it up eagerly (like unix). Otherwise, it looks to me like these might cause the accept callbacks to be run after the endgame had freed the memory for them. The comment here seems mixed up between send and recv buffers. The default behavior on calling `closesocket` is already to do a graceful shutdown (see https://docs.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-closesocket with default l_onoff=zero) if it is the last open handle. The expect behavior if there are pending reads in flight is to send an RST packet, notifying the client that the server connection was destroyed before acknowledging the EOF. Refs: libuv#3035 Refs: nodejs/node#35946 Refs: nodejs/node#35904 Fixes: libuv#3034 PR-URL:

This is an attempt to fix some resource management issues on Windows. Win32 sockets have an issue where it sends an RST packet if there is an outstanding overlapped calls. We can avoid that by being certain to explicitly cancel our read and write requests first. This also removes some conditional cleanup code, since we might as well clean it up eagerly (like unix). Otherwise, it looks to me like these might cause the accept callbacks to be run after the endgame had freed the memory for them. The comment here seems mixed up between send and recv buffers. The default behavior on calling `closesocket` is already to do a graceful shutdown (see https://docs.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-closesocket with default l_onoff=zero) if it is the last open handle. The expected behavior if there are pending reads in flight is to send an RST packet, notifying the client that the server connection was destroyed before acknowledging the EOF. Refs: libuv#3035 Refs: nodejs/node#35946 Refs: nodejs/node#35904 Fixes: libuv#3034 PR-URL: libuv#3036

RFC 5246 section-7.2.1 requires that the implementation must immediately stop using the stream, as it is no longer TLS-encrypted. The stream is permitted to still pump events (and errors) to other users, but those are now unencrypted, so we should not process them here. But therefore, we do not want to stop the underlying stream, as there could be another user of it, but we do remove ourselves as a listener. The section also states that the application must destroy the stream immediately (discarding any pending writes, and sending a close_notify response back), but we leave that to the upper layer of the application here, as it should be sufficient to permit standards compliant usage just to be ignoring read events. Fixes: nodejs/node#35904 Closes: nodejs/node#35946 Co-authored-by: Momtchil Momtchev <momtchil@momtchev.com>

mmomtchev · 2020-11-24T16:37:04Z

@vtjnash has a complete solution to this problem in a separate PR

RFC 5246 section-7.2.1 requires that the implementation must immediately stop reading from the stream, as it is no longer TLS-encrypted. The underlying stream is permitted to still pump events (and errors) to other users, but those are now unencrypted, so we should not process them here. But therefore, we do not want to stop the underlying stream, as there could be another user of it, but we do need to remove ourselves as a listener. Per TLS v1.2, we should have also destroy the TLS state entirely here (including the writing side), but this was revised in TLS v1.3 to permit the stream to continue to flush output. There appears to be some inconsistencies in the way nodejs handles ownership of the underlying stream, with `TLS.close()` on the write side also calling shutdown on the underlying stream (thus assuming other users of the underlying stream are not permitted), while receiving EOF on the read side leaves the underlying channel open. These inconsistencies are left for a later person to resolve, if the extra functionality is needed (as described in nodejs#35904). The current goal here is to the fix the occasional CI exceptions depending on the timing of these kernel messages through the TCP stack. Refs: libuv/libuv#3036 Refs: nodejs#35904 Closes: nodejs#35946 Co-authored-by: Momtchil Momtchev <momtchil@momtchev.com>

This is an attempt to fix some resource management issues on Windows. Win32 sockets have an issue where it sends an RST packet if there is an outstanding overlapped calls. We can avoid that by being certain to explicitly cancel our read and write requests first. This also removes some conditional cleanup code, since we might as well clean it up eagerly (like unix). Otherwise, it looks to me like these might cause the accept callbacks to be run after the endgame had freed the memory for them. The comment here seems mixed up between send and recv buffers. The default behavior on calling `closesocket` is already to do a graceful shutdown (see https://docs.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-closesocket with default l_onoff=zero) if it is the last open handle. The expected behavior if there are pending reads in flight is to send an RST packet, notifying the client that the server connection was destroyed before acknowledging the EOF. Additionally, we need to cancel writes explicitly: we need to notify Win32 that it is okay to cancel these writes (so it doesn't also generate an RST packet on the wire). Refs: #3035 Refs: nodejs/node#35946 Refs: nodejs/node#35904 Fixes: #3034 PR-URL: #3036 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

RFC 5246 section-7.2.1 requires that the implementation must immediately stop reading from the stream, as it is no longer TLS-encrypted. The underlying stream is permitted to still pump events (and errors) to other users, but those are now unencrypted, so we should not process them here. But therefore, we do not want to stop the underlying stream, as there could be another user of it, but we do need to remove ourselves as a listener. Per TLS v1.2, we should have also destroy the TLS state entirely here (including the writing side), but this was revised in TLS v1.3 to permit the stream to continue to flush output. There appears to be some inconsistencies in the way nodejs handles ownership of the underlying stream, with `TLS.close()` on the write side also calling shutdown on the underlying stream (thus assuming other users of the underlying stream are not permitted), while receiving EOF on the read side leaves the underlying channel open. These inconsistencies are left for a later person to resolve, if the extra functionality is needed (as described in nodejs#35904). The current goal here is to the fix the occasional CI exceptions depending on the timing of these kernel messages through the TCP stack. Refs: libuv/libuv#3036 Refs: nodejs#35904 Closes: nodejs#35946 Co-authored-by: Momtchil Momtchev <momtchil@momtchev.com>

RFC 5246 section-7.2.1 requires that the implementation must immediately stop reading from the stream, as it is no longer TLS-encrypted. The underlying stream is permitted to still pump events (and errors) to other users, but those are now unencrypted, so we should not process them here. But therefore, we do not want to stop the underlying stream, as there could be another user of it, but we do need to remove ourselves as a listener. Per TLS v1.2, we should have also destroy the TLS state entirely here (including the writing side), but this was revised in TLS v1.3 to permit the stream to continue to flush output. There appears to be some inconsistencies in the way nodejs handles ownership of the underlying stream, with `TLS.close()` on the write side also calling shutdown on the underlying stream (thus assuming other users of the underlying stream are not permitted), while receiving EOF on the read side leaves the underlying channel open. These inconsistencies are left for a later person to resolve, if the extra functionality is needed (as described in #35904). The current goal here is to the fix the occasional CI exceptions depending on the timing of these kernel messages through the TCP stack. PR-URL: #36111 Fixes: #35946 Refs: libuv/libuv#3036 Refs: #35904 Co-authored-by: Momtchil Momtchev <momtchil@momtchev.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

RFC 5246 section-7.2.1 requires that the implementation must immediately stop reading from the stream, as it is no longer TLS-encrypted. The underlying stream is permitted to still pump events (and errors) to other users, but those are now unencrypted, so we should not process them here. But therefore, we do not want to stop the underlying stream, as there could be another user of it, but we do need to remove ourselves as a listener. Per TLS v1.2, we should have also destroy the TLS state entirely here (including the writing side), but this was revised in TLS v1.3 to permit the stream to continue to flush output. There appears to be some inconsistencies in the way nodejs handles ownership of the underlying stream, with `TLS.close()` on the write side also calling shutdown on the underlying stream (thus assuming other users of the underlying stream are not permitted), while receiving EOF on the read side leaves the underlying channel open. These inconsistencies are left for a later person to resolve, if the extra functionality is needed (as described in nodejs#35904). The current goal here is to the fix the occasional CI exceptions depending on the timing of these kernel messages through the TCP stack. PR-URL: nodejs#36111 Fixes: nodejs#35946 Refs: libuv/libuv#3036 Refs: nodejs#35904 Co-authored-by: Momtchil Momtchev <momtchil@momtchev.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

RFC 5246 section-7.2.1 requires that the implementation must immediately stop reading from the stream, as it is no longer TLS-encrypted. The underlying stream is permitted to still pump events (and errors) to other users, but those are now unencrypted, so we should not process them here. But therefore, we do not want to stop the underlying stream, as there could be another user of it, but we do need to remove ourselves as a listener. Per TLS v1.2, we should have also destroy the TLS state entirely here (including the writing side), but this was revised in TLS v1.3 to permit the stream to continue to flush output. There appears to be some inconsistencies in the way nodejs handles ownership of the underlying stream, with `TLS.close()` on the write side also calling shutdown on the underlying stream (thus assuming other users of the underlying stream are not permitted), while receiving EOF on the read side leaves the underlying channel open. These inconsistencies are left for a later person to resolve, if the extra functionality is needed (as described in #35904). The current goal here is to the fix the occasional CI exceptions depending on the timing of these kernel messages through the TCP stack. PR-URL: #36111 Fixes: #35946 Refs: libuv/libuv#3036 Refs: #35904 Co-authored-by: Momtchil Momtchev <momtchil@momtchev.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

This is an attempt to fix some resource management issues on Windows. Win32 sockets have an issue where it sends an RST packet if there is an outstanding overlapped calls. We can avoid that by being certain to explicitly cancel our read and write requests first. This also removes some conditional cleanup code, since we might as well clean it up eagerly (like unix). Otherwise, it looks to me like these might cause the accept callbacks to be run after the endgame had freed the memory for them. The comment here seems mixed up between send and recv buffers. The default behavior on calling `closesocket` is already to do a graceful shutdown (see https://docs.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-closesocket with default l_onoff=zero) if it is the last open handle. The expected behavior if there are pending reads in flight is to send an RST packet, notifying the client that the server connection was destroyed before acknowledging the EOF. Additionally, we need to cancel writes explicitly: we need to notify Win32 that it is okay to cancel these writes (so it doesn't also generate an RST packet on the wire). Refs: libuv#3035 Refs: nodejs/node#35946 Refs: nodejs/node#35904 Fixes: libuv#3034 PR-URL: libuv#3036 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

This is an attempt to fix some resource management issues on Windows. Win32 sockets have an issue where it sends an RST packet if there is an outstanding overlapped calls. We can avoid that by being certain to explicitly cancel our read and write requests first. This also removes some conditional cleanup code, since we might as well clean it up eagerly (like unix). Otherwise, it looks to me like these might cause the accept callbacks to be run after the endgame had freed the memory for them. The comment here seems mixed up between send and recv buffers. The default behavior on calling `closesocket` is already to do a graceful shutdown (see https://docs.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-closesocket with default l_onoff=zero) if it is the last open handle. The expected behavior if there are pending reads in flight is to send an RST packet, notifying the client that the server connection was destroyed before acknowledging the EOF. Additionally, we need to cancel writes explicitly: we need to notify Win32 that it is okay to cancel these writes (so it doesn't also generate an RST packet on the wire). Refs: libuv/libuv#3035 Refs: nodejs/node#35946 Refs: nodejs/node#35904 Fixes: libuv/libuv#3034 PR-URL: libuv/libuv#3036 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

stream: remove kludge masking RST on Windows

1d26439

Remove the kludge that masks the TCP RST on Windows on test-https-truncate That RST is very real, originates from the server and should be investigated Fixes: nodejs#35904

ronag reviewed Nov 3, 2020

View reviewed changes

mmomtchev marked this pull request as draft November 3, 2020 18:01

http: graceful shutdown on res.end()

07219ab

On Windows and OSX, the HTTP server will often make the OS send a TCP RST packet by shutting down the socket before all incoming data has been read. Normally, libuv should take care of this (?), but it does not on Windows and OSX Refs: nodejs#35904

mmomtchev mentioned this pull request Nov 4, 2020

Closing a socket with a pending read makes Windows send a TCP RST packet libuv/libuv#3034

Closed

libuv: Delay closing the socket when read is pending

87baa75

On Windows, do not skip delaying the closesocket when the socket is not shared with another process - I don't see any reason which will allow this optimization Refs: nodejs#35946 Refs: libuv/libuv#3034

vtjnash mentioned this pull request Nov 13, 2020

TLS: improve compliance with shutdown standard, remove hacks #36111

Closed

lpinca mentioned this pull request Nov 23, 2020

http: replace destroySoon with socket.end #36205

Closed

2 tasks

mmomtchev closed this Nov 24, 2020

tniessen mentioned this pull request Mar 25, 2021

Node.js Technical Steering Committee (TSC) Meeting 2021-03-25 nodejs/TSC#999

Closed

octogonz mentioned this pull request Jul 5, 2022

stream: regression since v12.16.3 with TLS sockets backed by non-net.Socket streams #35904

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

stream: remove kludge masking RST on Windows #35946

stream: remove kludge masking RST on Windows #35946

Uh oh!

mmomtchev commented Nov 3, 2020

ronag left a comment

mmomtchev commented Nov 3, 2020

mmomtchev commented Nov 3, 2020

mmomtchev commented Nov 3, 2020

mmomtchev commented Nov 4, 2020

mmomtchev commented Nov 4, 2020

mmomtchev commented Nov 24, 2020

Labels

2 participants

Uh oh!

stream: remove kludge masking RST on Windows #35946

stream: remove kludge masking RST on Windows #35946

Uh oh!

Conversation

mmomtchev commented Nov 3, 2020

Checklist

ronag left a comment

Choose a reason for hiding this comment

mmomtchev commented Nov 3, 2020

mmomtchev commented Nov 3, 2020

mmomtchev commented Nov 3, 2020

mmomtchev commented Nov 4, 2020

mmomtchev commented Nov 4, 2020

mmomtchev commented Nov 24, 2020

Labels

2 participants