http_outgoing: refactor to use stream.Writable #6085
Conversation
|
cc @nodejs/http will also fix cc @trevnorris not sure how fast it is 😢 . Perhaps you could help us? |
|
and cc @bnoordhuis because you are always upset at my patches |
indutny
added
http
|
I have added semver-major label to this PR, however I don't think that it should go into v6. I would give it a try in master though, and, perhaps, several minor releases of v7 before moving it to the next LTS |
| @@ -42,8 +42,7 @@ server.listen(common.PORT, function() { | |||
| break; | |||
| } | |||
|
|
|||
| assert.equal(req.output.length, 0); | |||
| assert.equal(req.outputEncodings.length, 0); | |||
| assert.equal(req._writableState.length, 0); | |||
There was a problem hiding this comment.
No more .output and .outputEncodings
|
One more try CI: https://ci.nodejs.org/job/node-test-pull-request/2239/ |
|
Gosh, looks like this patch needs some serious performance improvements. Will see what I can do about it. |
Use `stream.Writable` instead of the piles of suspicious methods on the top of the legacy `Stream`. Additionally, make `res.write()` return the same value as regular `stream.Writable#write` would do after `res.end()` call. Returning `true` is considered a blasphemy from now on, and should be prosecuted with all possible strictness.
indutny
force-pushed
the
feature/stream-response
branch
from
1913bf8
to
302519f
Compare
estliberitas
force-pushed
the
master
branch
2 times, most recently
from
7da4fd4
to
c7066fb
Compare
| this.socket.destroy(error); | ||
| else | ||
| this.once('socket', function(socket) { | ||
| const socket = this.socket; |
There was a problem hiding this comment.
In my experience (which may be outdated) this provides no (performance) benefit, nor do you explicitly need it here.
There was a problem hiding this comment.
The only benefit that I thought about is character count :) There are at least four occurrences of socket down there.
There was a problem hiding this comment.
There may be performance difference, though, but in this particular function it is definitely neglectable.
There was a problem hiding this comment.
Yeah, I don't think perf impact would be negative. Just thought you may be trying to optimize for speed here. Carry on :)
|
@ronkorving thank you so much for reviewing this! I just wanted to let you know that this PR is in unfinished state, so please be aware that many things may change. I still need to optimize it performance-wise to make it as fast as the current version (if it is possible at all). |
|
No problem, I'm just going through some PRs that seem to be in limbo or in-progress and provide the feedback I can :) Good luck. |
rvagg
force-pushed
the
master
branch
2 times, most recently
from
c133999
to
83c7a88
Compare
|
There was no update for a long while, closing this therefore. @indutny please just reopen if you want to continue working on it. |
Checklist
Affected core subsystem(s)
http
Description of change
Use
stream.Writableinstead of the piles of suspicious methods on thetop of the legacy
Stream.Additionally, make
res.write()return the same value as regularstream.Writable#writewould do afterres.end()call. Returningtrueis considered a blasphemy from now on, and should be prosecutedwith all possible strictness.