FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http: outgoing cork by ronag · Pull Request #29053 · nodejs/node · GitHub

/ node Public

http: outgoing cork - #29053

Closed
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:http-outgoing-cork
ConversationCommits (1)ChecksFiles changed
Closed

http: outgoing cork#29053
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:http-outgoing-cork

Conversation

ronag commented Aug 8, 2019

Copy link
Copy Markdown
Member

Implements cork/uncork on OutgoingMessage to make it more streamlike.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Aug 8, 2019
ronag mentioned this pull request Aug 8, 2019
Closed
9 tasks
ronag force-pushed the http-outgoing-cork branch 3 times, most recently from 961eefa to 0ddf57b Compare August 8, 2019 19:22

ronag commented Aug 8, 2019

Copy link
Copy Markdown
Member Author

Not sure exactly how corking a http2 stream works. Would that cork other responses as well? Or does the multiplexing happen on a lower level?

mscdex reviewed Aug 8, 2019
View reviewed changes
Comment thread doc/api/http.md Outdated
ronag force-pushed the http-outgoing-cork branch 3 times, most recently from 92fa16b to d54f0a5 Compare August 9, 2019 16:00

jasnell commented Aug 22, 2019

Copy link
Copy Markdown
Member

multiplexing happens on the native layer. corking an Http2Stream has no impact on any other Http2Stream instances.

addaleax approved these changes Nov 3, 2019
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, but this needs a rebase

ronag force-pushed the http-outgoing-cork branch 3 times, most recently from 650eeab to c650a63 Compare November 3, 2019 22:27
addaleax reviewed Nov 4, 2019
View reviewed changes
Comment thread lib/_http_outgoing.js Outdated
ronag force-pushed the http-outgoing-cork branch from c650a63 to 796f541 Compare November 4, 2019 08:59

nodejs-github-bot commented Nov 4, 2019

Copy link
Copy Markdown
Collaborator

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

ronag commented Nov 4, 2019

Copy link
Copy Markdown
Member Author

Sorry, there is a test failure. Right now I can't build node due to:

No Xcode or CLT version detected!

I'll fix this once I've figured out how to build Node again.

ronag commented Nov 4, 2019

Copy link
Copy Markdown
Member Author

@addaleax: It might be a good idea to merge this first #29012

addaleax commented Nov 5, 2019

Copy link
Copy Markdown
Member

@addaleax: It might be a good idea to merge this first #29012

@ronag Done, but now this needs a rebase again 😅

ronag force-pushed the http-outgoing-cork branch from 796f541 to 7290bf1 Compare November 6, 2019 23:44

ronag commented Nov 6, 2019

Copy link
Copy Markdown
Member Author

rebased

ronag commented Nov 11, 2019

Copy link
Copy Markdown
Member Author

@Trott failed on compile V8?

nodejs-github-bot commented Nov 11, 2019

Copy link
Copy Markdown
Collaborator

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

Trott commented Nov 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Test failures in test-http-response-cork:

NaN !== undefined

    at Server.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/alpine-latest-x64/test/parallel/test-http-response-cork.js:15:10)

ronag force-pushed the http-outgoing-cork branch from 7290bf1 to b5a611f Compare November 12, 2019 13:51

ronag commented Nov 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

Hm, that was a minor bug in Duplex. Resolved as well. @Trott

nodejs-github-bot commented Nov 12, 2019

Copy link
Copy Markdown
Collaborator

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

Trott commented Nov 12, 2019

Copy link
Copy Markdown
Member

@nodejs/http This could use more reviews.

addaleax approved these changes Nov 13, 2019
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM but I’d put the _stream_duplex.js changes in a separate commit (while landing)

addaleax added a commit that referenced this pull request Nov 13, 2019
stream: add writableCorked to Duplex
dc7b5fc 
PR-URL: #29053
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request Nov 13, 2019
http: outgoing cork
f3ea4e9 
PR-URL: #29053
Reviewed-By: Anna Henningsen <anna@addaleax.net>

addaleax commented Nov 13, 2019

Copy link
Copy Markdown
Member

Landed in dc7b5fc f3ea4e9, thank you for remaining committed to getting this done :)

addaleax closed this Nov 13, 2019
MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
stream: add writableCorked to Duplex
41720d7 
PR-URL: #29053
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
http: outgoing cork
62e15a7 
PR-URL: #29053
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BridgeAR mentioned this pull request Nov 19, 2019
Merged
addaleax mentioned this pull request Nov 30, 2019
Closed
4 tasks
targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 1, 2019
targos pushed a commit that referenced this pull request Jan 13, 2020
stream: add writableCorked to Duplex
63e3a62 
PR-URL: #29053
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Jan 13, 2020
http: outgoing cork
c1b73f2 
PR-URL: #29053
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
stream: add writableCorked to Duplex
55ca3a8 
PR-URL: #29053
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
http: outgoing cork
db8144b 
PR-URL: #29053
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins mentioned this pull request Feb 8, 2020
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax approved these changes
+1 more reviewer
mscdex mscdex left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

http Issues or PRs related to the http subsystem. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL