http: Make OutgoingMessage more streamlike #45672
Merged
Conversation
This file contains 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
|
Review requested:
|
nodejs-github-bot
added
http
ronag
force-pushed
the
outgoing-writable-12
branch
3 times, most recently
from
0af46f6
to
ca3ed8a
Compare
Implement missing getters error & closed. Add support for proper "writable" check through isWritable helper. We cannot fix the OutgoingMessage.writable propery as that will break the ecosystem.
ronag
force-pushed
the
outgoing-writable-12
branch
from
ca3ed8a
to
1047db4
Compare
aduh95
reviewed
Nov 29, 2022
|
The changes LGTM. Are you going to add any test? |
ronag
added
request-ci
github-actions
bot
removed
the
request-ci
15 tasks
aduh95
reviewed
Nov 30, 2022
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
github-actions
bot
removed
the
request-ci
10 tasks
Flarna
approved these changes
Dec 1, 2022
10 tasks
JungMinu
approved these changes
Dec 2, 2022
aduh95
approved these changes
Dec 2, 2022
|
Landed in d385106 |
This was referenced Dec 3, 2022
targos
pushed a commit
that referenced
this pull request
Dec 12, 2022
Implement missing getters error & closed. Add support for proper "writable" check through `isWritable` helper. We cannot fix the `OutgoingMessage.writable` property as that would break the ecosystem. PR-URL: #45672 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams
pushed a commit
that referenced
this pull request
Dec 30, 2022
Implement missing getters error & closed. Add support for proper "writable" check through `isWritable` helper. We cannot fix the `OutgoingMessage.writable` property as that would break the ecosystem. PR-URL: #45672 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams
pushed a commit
that referenced
this pull request
Dec 30, 2022
Implement missing getters error & closed. Add support for proper "writable" check through `isWritable` helper. We cannot fix the `OutgoingMessage.writable` property as that would break the ecosystem. PR-URL: #45672 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams
pushed a commit
that referenced
this pull request
Jan 3, 2023
Implement missing getters error & closed. Add support for proper "writable" check through `isWritable` helper. We cannot fix the `OutgoingMessage.writable` property as that would break the ecosystem. PR-URL: #45672 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams
pushed a commit
that referenced
this pull request
Jan 4, 2023
Implement missing getters error & closed. Add support for proper "writable" check through `isWritable` helper. We cannot fix the `OutgoingMessage.writable` property as that would break the ecosystem. PR-URL: #45672 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams
pushed a commit
that referenced
this pull request
Jan 5, 2023
Implement missing getters error & closed. Add support for proper "writable" check through `isWritable` helper. We cannot fix the `OutgoingMessage.writable` property as that would break the ecosystem. PR-URL: #45672 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
ijjk
pushed a commit
to vercel/next.js
that referenced
this pull request
Jul 26, 2023
### What?
This reimplements our stream cancellation code for a few more cases:
1. Adds support in all stream-returning APIs
2. Fixes cancellation detection in node 16
3. Implements out-of-band detection, so can cancel in the middle of a
read
It also (finally) adds tests for all the cases I'm aware of.
### Why?
To allow disconnecting from an AI service when a client disconnects. $$$
### How?
1. Reuses a single pipe function in all paths to push data from the
dev's `ReadableStream` into our `ServerResponse`
2. Uses `ServerResponse` to detect disconnect, instead of the
`IncomingMessage` (request)
- The `close` event fire once all incoming body data is read
- The request `abort` event will not fire after the incoming body data
has been fully read
3. Using `on('close')` on the writable destination allows us to detect
close
- Checking for `res.destroyed` in the body of the loop meant we had to
wait for the `await stream.read()` to complete before we could possibly
cancel the stream
- - -
#52157 (and #51594) had an issue with Node 16, because I was using
`res.closed` to detect when the server response was closed by the client
disconnecting. But, `closed` wasn't
[added](nodejs/node#45672) until
[v18.13.0](https://nodejs.org/en/blog/release/v18.13.0#:~:text=%5Bcbd710bbf4%5D%20%2D%20http%3A%20make%20OutgoingMessage%20more%20streamlike%20(Robert%20Nagy)%20%2345672).
This fixes it by using `res.destroyed`.
Reverts #52277
Relands #52157
Fixes #52809
---------
Strift
pushed a commit
to Strift/next.js
that referenced
this pull request
Jul 27, 2023
### What?
This reimplements our stream cancellation code for a few more cases:
1. Adds support in all stream-returning APIs
2. Fixes cancellation detection in node 16
3. Implements out-of-band detection, so can cancel in the middle of a
read
It also (finally) adds tests for all the cases I'm aware of.
### Why?
To allow disconnecting from an AI service when a client disconnects. $$$
### How?
1. Reuses a single pipe function in all paths to push data from the
dev's `ReadableStream` into our `ServerResponse`
2. Uses `ServerResponse` to detect disconnect, instead of the
`IncomingMessage` (request)
- The `close` event fire once all incoming body data is read
- The request `abort` event will not fire after the incoming body data
has been fully read
3. Using `on('close')` on the writable destination allows us to detect
close
- Checking for `res.destroyed` in the body of the loop meant we had to
wait for the `await stream.read()` to complete before we could possibly
cancel the stream
- - -
vercel#52157 (and vercel#51594) had an issue with Node 16, because I was using
`res.closed` to detect when the server response was closed by the client
disconnecting. But, `closed` wasn't
[added](nodejs/node#45672) until
[v18.13.0](https://nodejs.org/en/blog/release/v18.13.0#:~:text=%5Bcbd710bbf4%5D%20%2D%20http%3A%20make%20OutgoingMessage%20more%20streamlike%20(Robert%20Nagy)%20%2345672).
This fixes it by using `res.destroyed`.
Reverts vercel#52277
Relands vercel#52157
Fixes vercel#52809
---------
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implement missing getters error & closed. Add support for proper "writable" check through isWritable helper.
We cannot fix the OutgoingMessage.writable propery as that will break the ecosystem.