GeezerVison

Copy link

Copy Markdown

Member

bricss reacted with thumbs up emoji

ronag requested a review from aduh95 November 29, 2022 10:43

ronag requested a review from mcollina November 29, 2022 10:45

ronag force-pushed the outgoing-writable-12 branch 3 times, most recently from 0af46f6 to ca3ed8a Compare November 29, 2022 10:46

ronag force-pushed the outgoing-writable-12 branch from ca3ed8a to 1047db4 Compare November 29, 2022 10:46

aduh95 reviewed Nov 29, 2022

View reviewed changes

mcollina approved these changes Nov 30, 2022

View reviewed changes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aduh95 reviewed Nov 30, 2022

View reviewed changes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Flarna approved these changes Dec 1, 2022

View reviewed changes

JungMinu approved these changes Dec 2, 2022

View reviewed changes

aduh95 approved these changes Dec 2, 2022

View reviewed changes

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 <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

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 <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

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 <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

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 <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

### 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

---------

### 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

---------