Skip to content
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.

Sign up for GitHub

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

Jump to bottom

http: Make OutgoingMessage more streamlike #45672

Merged
merged 10 commits into from
Dec 2, 2022
Merged

http: Make OutgoingMessage more streamlike #45672

merged 10 commits into from
Dec 2, 2022

Conversation

ronag
Copy link
Member

@ronag ronag commented Nov 29, 2022

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 ronag requested a review from aduh95 November 29, 2022 10:43
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net
  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Nov 29, 2022
@ronag ronag requested a review from mcollina November 29, 2022 10:45
@ronag ronag force-pushed the outgoing-writable-12 branch 3 times, most recently from 0af46f6 to ca3ed8a Compare November 29, 2022 10:46
@ronag
http: Make OutgoingMessage more streamlike
1047db4 
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.
@ShogunPanda
Copy link
Contributor

The changes LGTM. Are you going to add any test?

@ronag ronag added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Nov 29, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 29, 2022
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/48234/

@github-actions github-actions bot mentioned this pull request Nov 30, 2022
Open
15 tasks
mcollina
mcollina approved these changes Nov 30, 2022
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

aduh95
aduh95 reviewed Nov 30, 2022
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are seemingly related test failures.

lib/_http_outgoing.js Outdated Show resolved Hide resolved
lib/_http_outgoing.js Outdated Show resolved Hide resolved
test/parallel/test-http-client-incomingmessage-destroy.js Outdated Show resolved Hide resolved
test/parallel/test-http-client-incomingmessage-destroy.js Outdated Show resolved Hide resolved
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 30, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 30, 2022
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/48246/

@github-actions github-actions bot mentioned this pull request Dec 1, 2022
Open
10 tasks
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/48260/

@github-actions github-actions bot mentioned this pull request Dec 2, 2022
Open
10 tasks
@aduh95 aduh95 merged commit d385106 into nodejs:main Dec 2, 2022
@aduh95
Copy link
Contributor

aduh95 commented Dec 2, 2022

Landed in d385106

This was referenced Dec 3, 2022
Open
Open
Open
Open
Open
targos pushed a commit that referenced this pull request Dec 12, 2022
@ronag @targos
http: make OutgoingMessage more streamlike
d64e773 
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]>
@targos targos mentioned this pull request Dec 12, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@ronag @danielleadams
http: make OutgoingMessage more streamlike
da35754 
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]>
@danielleadams danielleadams mentioned this pull request Dec 30, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@ronag @danielleadams
http: make OutgoingMessage more streamlike
5a1745e 
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]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
@ronag @danielleadams
http: make OutgoingMessage more streamlike
cbd710b 
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]>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
@ronag @danielleadams
http: make OutgoingMessage more streamlike
9db2ec6 
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]>
danielleadams pushed a commit that referenced this pull request Jan 5, 2023
@ronag @danielleadams
http: make OutgoingMessage more streamlike
9025d6c 
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]>
@jridgewell jridgewell mentioned this pull request Jul 6, 2023
Merged
ijjk pushed a commit to vercel/next.js that referenced this pull request Jul 26, 2023
@jridgewell
Reimplement stream cancellation (#52281)
31d2b72 
### 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
@jridgewell @Strift
Reimplement stream cancellation (vercel#52281)
0117968 
### 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
Reviewers

@Flarna Flarna Flarna approved these changes

@aduh95 aduh95 aduh95 approved these changes

@JungMinu JungMinu JungMinu approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ronag @nodejs-github-bot @ShogunPanda @aduh95 @mcollina @JungMinu @Flarna