http: align with stream.Writable by ronag · Pull Request #36816 · nodejs/node · GitHub

Futher aligns OutgoingMessage with stream.Writable. In particular
re-uses the construct/destroy logic from streams.

Due to a lot of subtle assumptions this PR unfortunately touches
a lot of different parts.

@nodejs/http

I've removed the following tests:

test/parallel/test-http-agent-uninitialized-with-handle.js
test/parallel/test-http-agent-uninitialized.js

They fail and I'm confused as to what they are supposed to test and how they could work.

I've removed the following tests:

test/parallel/test-http-agent-uninitialized-with-handle.js

test/parallel/test-http-agent-uninitialized.js

They fail and I'm confused as to what they are supposed to test and how they could work

From 93f47b1#diff-82c531677b9ab4e260025e0397fb0ac4d4316459fa5d16346324e39584cfce96R170 , it looks like they're trying to check that Agent#addRequest does not break if the socket does not have a _handle. The comments say that it could happen if the socket is uninitialized or supplied by the user. I am not sure how this could happen in a use case scenario. What are the errors you were encountering in the tests?

From 93f47b1#diff-82c531677b9ab4e260025e0397fb0ac4d4316459fa5d16346324e39584cfce96R170 , it looks like they're trying to check that Agent#addRequest does not break if the socket does not have a _handle. The comments say that it could happen if the socket is uninitialized or supplied by the user. I am not sure how this could happen in a use case scenario. What are the errors you were encountering in the tests?

Would you like to have a go at trying to make those test pass?

@nodejs/http any chance to get some review on this PR?

It looks like this needs TSC approvals to land and the TSC ping didn't help - pinging @nodejs/tsc

This looks good to me and I'll approve it if it ends up one TSC approval short of landing or something like that. But for things that are at the intersection of http and streams, I tend to wait for @mcollina and/or @lpinca to review first.

My biggest concern here is that this is a pretty "soft" alignment. It's done through explicitly modifying the logic and flows to mirror stream.Writable. That is fine but it does introduce additional future maintenance burden and more opportunity for weird, unexpected divergence that is less of an issue the way things are setup right now. To be clear, this is not a criticism of the approach — I understand why it's done the way it is, it's more pointing out that this does create shared behavior through code duplication.

At the very least, I think some new tests should be introduced that document expected behavior and assumptions that can be made about http.OutgoingMessage that is more closely aligned with stream.Writable. In particular I think we need tests that document the introduction of _writableState and Writable.prototype in a variety of places. That's just my 2c but I'm also more risk-averse than most when it comes to the http module.

That is fine but it does introduce additional future maintenance burden and more opportunity for weird, unexpected divergence that is less of an issue the way things are setup right now.

I'm not sure I follow this line of thought? IMHO These changes should make it easier to maintain this and all stream related helpers and interop?

IMHO There changes should make it easier to maintain this and all stream related helpers and interop?

The closer you get while still having custom logic & duplicate code, the harder it is to not accidentally diverge in some minute way. At least without having equivalent test suites for both.

FWIW don't take this as me saying this is a bad change. I think it's great we're moving in this direction, although I would prefer to better align test coverage with Writable.

FWIW my intention here is to eventually implement it fully in terms of Writable (or some form of WritableBase).

Sorry for not looking at this sooner. The changes themselves look fine to me but I'd like @mcollina to weigh in before signing off.

LGTM with caution. We might end up to revert this if it will lead to massive ecosystem breakage.

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

Should we label this as a notable change?

Yes indeed

Futher aligns OutgoingMessage with stream.Writable. In particular re-uses the construct/destroy logic from streams. Due to a lot of subtle assumptions this PR unfortunately touches a lot of different parts.

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

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

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

Landed in e2f5bb7

Futher aligns OutgoingMessage with stream.Writable. In particular re-uses the construct/destroy logic from streams. Due to a lot of subtle assumptions this PR unfortunately touches a lot of different parts. PR-URL: #36816 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

ronag added http Issues or PRs related to the http subsystem. stream Issues and PRs related to the stream subsystem. labels Jan 6, 2021

ronag requested review from jasnell, mcollina and rickyes January 6, 2021 17:15

ronag commented Jan 6, 2021

View reviewed changes

Comment thread test/parallel/test-http-agent-uninitialized-with-handle.js Outdated

ronag commented Jan 6, 2021

View reviewed changes

Comment thread test/parallel/test-http-agent-uninitialized.js Outdated

mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 6, 2021

ronag force-pushed the http-outgoing branch from 010615c to 989c566 Compare January 6, 2021 18:23

ronag commented Jan 6, 2021

View reviewed changes

Comment thread test/parallel/test-http-outgoing-end-multiple.js Outdated

ronag requested review from addaleax and lpinca January 6, 2021 19:15

ronag force-pushed the http-outgoing branch from ace91a8 to 34bf0bb Compare January 7, 2021 07:07

ronag force-pushed the http-outgoing branch 2 times, most recently from d1a8699 to 60d2417 Compare January 7, 2021 08:13

ronag added the wip Issues and PRs that are still a work in progress. label Jan 7, 2021

ronag force-pushed the http-outgoing branch 2 times, most recently from b7bdc6a to c18b3b4 Compare January 7, 2021 08:20

dnlup reviewed Jan 7, 2021

View reviewed changes

Comment thread lib/_http_client.js Outdated

ronag force-pushed the http-outgoing branch 4 times, most recently from ba8188e to 09f6982 Compare January 7, 2021 11:04

ronag force-pushed the http-outgoing branch from 5aeab87 to 09f6982 Compare January 7, 2021 11:26

ronag commented Jan 7, 2021

View reviewed changes

Comment thread test/parallel/test-stream-pipeline.js Outdated

ronag added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jan 31, 2021

benjamingr added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Feb 9, 2021

mhdawson mentioned this pull request Feb 15, 2021

Node.js Technical Steering Committee (TSC) Meeting 2021-02-18 nodejs/TSC#971

Closed

mhdawson mentioned this pull request Feb 23, 2021

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

Closed

mhdawson mentioned this pull request Mar 1, 2021

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

Closed

mhdawson mentioned this pull request Mar 8, 2021

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

Closed

mcollina approved these changes Mar 8, 2021

View reviewed changes

mcollina added the notable-change PRs with changes that should be highlighted in changelogs. label Mar 8, 2021

http: align with stream.Writable

107d201

Futher aligns OutgoingMessage with stream.Writable. In particular re-uses the construct/destroy logic from streams. Due to a lot of subtle assumptions this PR unfortunately touches a lot of different parts.

ronag force-pushed the http-outgoing branch from 30f5772 to 107d201 Compare March 8, 2021 20:41

benjamingr approved these changes Mar 9, 2021

View reviewed changes

ronag closed this Mar 10, 2021

BethGriggs mentioned this pull request Mar 19, 2021

2021-04-20, Version 16.0.0 (Current) #37678

Merged

Linkgoron mentioned this pull request Mar 27, 2021

http: significant performance regression on master #37937

Closed

3 tasks

http: align with stream.Writable by ronag · Pull Request #36816 · nodejs/node · GitHub

Uh oh!

Uh oh!

http: align with stream.Writable - #36816

http: align with stream.Writable#36816ronag wants to merge 1 commit intonodejs:masternodejs/node:masterfrom nxtedition:http-outgoingnxtedition/node:http-outgoingCopy head branch name to clipboard

Conversation

ronag commented Jan 6, 2021

Uh oh!

Uh oh!

Uh oh!

ronag commented Jan 6, 2021

Uh oh!

Uh oh!

ronag commented Jan 6, 2021

Uh oh!

ronag commented Jan 6, 2021

Uh oh!

dnlup commented Jan 7, 2021

Uh oh!

Uh oh!

ronag commented Jan 7, 2021

Uh oh!

Uh oh!

ronag commented Feb 5, 2021

Uh oh!

benjamingr commented Feb 9, 2021

Uh oh!

Trott commented Feb 14, 2021

Uh oh!

apapirovski commented Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apapirovski commented Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Feb 25, 2021

Uh oh!

jasnell commented Mar 5, 2021

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Mar 8, 2021

Uh oh!

ronag commented Mar 8, 2021

Uh oh!

mcollina commented Mar 8, 2021

Uh oh!

nodejs-github-bot commented Mar 8, 2021

Uh oh!

nodejs-github-bot commented Mar 8, 2021

Uh oh!

nodejs-github-bot commented Mar 8, 2021

Uh oh!

ronag commented Mar 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

http: align with stream.Writable#36816
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:http-outgoing

apapirovski commented Feb 25, 2021 •
edited
Loading

ronag commented Feb 25, 2021 •
edited
Loading

apapirovski commented Feb 25, 2021 •
edited
Loading