http: align with stream.Writable

[ronag]

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]

@nodejs/http

[ronag]

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.

[ronag]

@dnlup

[dnlup]

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?

[ronag]

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?

[ronag]

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

[benjamingr]

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

[Trott]

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.

[apapirovski]

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.

[ronag]

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?

[apapirovski]

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.

[ronag]

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

[jasnell]

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.

[mcollina]

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

[nodejs-github-bot]

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

[ronag]

Should we label this as a notable change?

[mcollina]

Yes indeed

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[ronag]

Landed in e2f5bb7