http2: fix graceful session close by lundibundi · Pull Request #30854 · nodejs/node
nodejs-github-bot
added
c++
labels
Dec 8, 2019This slightly alters the behaviour of session close by first using .end() on a session socket to finish writing the data and only then calls .destroy() to make sure the Readable side is closed. This allows the socket to finish transmitting data, receive proper FIN packet and avoid ECONNRESET errors upon graceful close. onStreamClose now directly calls stream.destroy() instead of kMaybeDestroy because the latter will first check that the stream has writableFinished set. And that may not be true as we have just (synchronously) called .end() on the stream if it was not closed and that doesn't give it enough time to finish. Furthermore there is no point in waiting for 'finish' as the other party have already closed the stream and we won't be able to write anyway. This also changes a few tests to correctly handle graceful session close. This includes: * not reading request data (on client side) * not reading push stream data (on client side) * relying on socket.destroy() (on client) to finish server session due to the destroy of the socket without closing the server session. As the goaway itself is *not* a session close. Added few 'close' event mustCall checks.
lundibundi
added
the
author ready
label
Dec 21, 2019BridgeAR pushed a commit that referenced this pull request
Dec 25, 2019PR-URL: #30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR pushed a commit that referenced this pull request
Dec 25, 2019This slightly alters the behaviour of session close by first using .end() on a session socket to finish writing the data and only then calls .destroy() to make sure the Readable side is closed. This allows the socket to finish transmitting data, receive proper FIN packet and avoid ECONNRESET errors upon graceful close. onStreamClose now directly calls stream.destroy() instead of kMaybeDestroy because the latter will first check that the stream has writableFinished set. And that may not be true as we have just (synchronously) called .end() on the stream if it was not closed and that doesn't give it enough time to finish. Furthermore there is no point in waiting for 'finish' as the other party have already closed the stream and we won't be able to write anyway. This also changes a few tests to correctly handle graceful session close. This includes: * not reading request data (on client side) * not reading push stream data (on client side) * relying on socket.destroy() (on client) to finish server session due to the destroy of the socket without closing the server session. As the goaway itself is *not* a session close. Added few 'close' event mustCall checks. PR-URL: #30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR pushed a commit that referenced this pull request
Jan 3, 2020PR-URL: #30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR pushed a commit that referenced this pull request
Jan 3, 2020This slightly alters the behaviour of session close by first using .end() on a session socket to finish writing the data and only then calls .destroy() to make sure the Readable side is closed. This allows the socket to finish transmitting data, receive proper FIN packet and avoid ECONNRESET errors upon graceful close. onStreamClose now directly calls stream.destroy() instead of kMaybeDestroy because the latter will first check that the stream has writableFinished set. And that may not be true as we have just (synchronously) called .end() on the stream if it was not closed and that doesn't give it enough time to finish. Furthermore there is no point in waiting for 'finish' as the other party have already closed the stream and we won't be able to write anyway. This also changes a few tests to correctly handle graceful session close. This includes: * not reading request data (on client side) * not reading push stream data (on client side) * relying on socket.destroy() (on client) to finish server session due to the destroy of the socket without closing the server session. As the goaway itself is *not* a session close. Added few 'close' event mustCall checks. PR-URL: #30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request
Jan 14, 2020PR-URL: #30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request
Jan 14, 2020This slightly alters the behaviour of session close by first using .end() on a session socket to finish writing the data and only then calls .destroy() to make sure the Readable side is closed. This allows the socket to finish transmitting data, receive proper FIN packet and avoid ECONNRESET errors upon graceful close. onStreamClose now directly calls stream.destroy() instead of kMaybeDestroy because the latter will first check that the stream has writableFinished set. And that may not be true as we have just (synchronously) called .end() on the stream if it was not closed and that doesn't give it enough time to finish. Furthermore there is no point in waiting for 'finish' as the other party have already closed the stream and we won't be able to write anyway. This also changes a few tests to correctly handle graceful session close. This includes: * not reading request data (on client side) * not reading push stream data (on client side) * relying on socket.destroy() (on client) to finish server session due to the destroy of the socket without closing the server session. As the goaway itself is *not* a session close. Added few 'close' event mustCall checks. PR-URL: #30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit to targos/node that referenced this pull request
Apr 25, 2020PR-URL: nodejs#30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit to targos/node that referenced this pull request
Apr 25, 2020This slightly alters the behaviour of session close by first using .end() on a session socket to finish writing the data and only then calls .destroy() to make sure the Readable side is closed. This allows the socket to finish transmitting data, receive proper FIN packet and avoid ECONNRESET errors upon graceful close. onStreamClose now directly calls stream.destroy() instead of kMaybeDestroy because the latter will first check that the stream has writableFinished set. And that may not be true as we have just (synchronously) called .end() on the stream if it was not closed and that doesn't give it enough time to finish. Furthermore there is no point in waiting for 'finish' as the other party have already closed the stream and we won't be able to write anyway. This also changes a few tests to correctly handle graceful session close. This includes: * not reading request data (on client side) * not reading push stream data (on client side) * relying on socket.destroy() (on client) to finish server session due to the destroy of the socket without closing the server session. As the goaway itself is *not* a session close. Added few 'close' event mustCall checks. PR-URL: nodejs#30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
clshortfuse pushed a commit to clshortfuse/node that referenced this pull request
Sep 22, 2020PR-URL: nodejs#30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
clshortfuse pushed a commit to clshortfuse/node that referenced this pull request
Sep 22, 2020This slightly alters the behaviour of session close by first using .end() on a session socket to finish writing the data and only then calls .destroy() to make sure the Readable side is closed. This allows the socket to finish transmitting data, receive proper FIN packet and avoid ECONNRESET errors upon graceful close. onStreamClose now directly calls stream.destroy() instead of kMaybeDestroy because the latter will first check that the stream has writableFinished set. And that may not be true as we have just (synchronously) called .end() on the stream if it was not closed and that doesn't give it enough time to finish. Furthermore there is no point in waiting for 'finish' as the other party have already closed the stream and we won't be able to write anyway. This also changes a few tests to correctly handle graceful session close. This includes: * not reading request data (on client side) * not reading push stream data (on client side) * relying on socket.destroy() (on client) to finish server session due to the destroy of the socket without closing the server session. As the goaway itself is *not* a session close. Added few 'close' event mustCall checks. PR-URL: nodejs#30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request
Oct 13, 2020MylesBorins pushed a commit that referenced this pull request
Oct 13, 2020This slightly alters the behaviour of session close by first using .end() on a session socket to finish writing the data and only then calls .destroy() to make sure the Readable side is closed. This allows the socket to finish transmitting data, receive proper FIN packet and avoid ECONNRESET errors upon graceful close. onStreamClose now directly calls stream.destroy() instead of kMaybeDestroy because the latter will first check that the stream has writableFinished set. And that may not be true as we have just (synchronously) called .end() on the stream if it was not closed and that doesn't give it enough time to finish. Furthermore there is no point in waiting for 'finish' as the other party have already closed the stream and we won't be able to write anyway. This also changes a few tests to correctly handle graceful session close. This includes: * not reading request data (on client side) * not reading push stream data (on client side) * relying on socket.destroy() (on client) to finish server session due to the destroy of the socket without closing the server session. As the goaway itself is *not* a session close. Added few 'close' event mustCall checks. Backport-PR-URL: #34845 PR-URL: #30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request
Nov 16, 2020MylesBorins pushed a commit that referenced this pull request
Nov 16, 2020This slightly alters the behaviour of session close by first using .end() on a session socket to finish writing the data and only then calls .destroy() to make sure the Readable side is closed. This allows the socket to finish transmitting data, receive proper FIN packet and avoid ECONNRESET errors upon graceful close. onStreamClose now directly calls stream.destroy() instead of kMaybeDestroy because the latter will first check that the stream has writableFinished set. And that may not be true as we have just (synchronously) called .end() on the stream if it was not closed and that doesn't give it enough time to finish. Furthermore there is no point in waiting for 'finish' as the other party have already closed the stream and we won't be able to write anyway. This also changes a few tests to correctly handle graceful session close. This includes: * not reading request data (on client side) * not reading push stream data (on client side) * relying on socket.destroy() (on client) to finish server session due to the destroy of the socket without closing the server session. As the goaway itself is *not* a session close. Added few 'close' event mustCall checks. Backport-PR-URL: #34845 PR-URL: #30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters