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.

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

http: outgoing cork #29053

Closed
wants to merge 1 commit into from
Closed

Conversation

ronag
Copy link
Member

@ronag ronag commented Aug 8, 2019

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 nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Aug 8, 2019
@ronag ronag mentioned this pull request Aug 8, 2019
9 tasks
@ronag ronag force-pushed the http-outgoing-cork branch 3 times, most recently from 961eefa to 0ddf57b Compare August 8, 2019 19:22
@ronag
Copy link
Member Author

ronag commented Aug 8, 2019

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?

doc/api/http.md Outdated Show resolved Hide resolved
@ronag ronag force-pushed the http-outgoing-cork branch 3 times, most recently from 92fa16b to d54f0a5 Compare August 9, 2019 16:00
@jasnell
Copy link
Member

jasnell commented Aug 22, 2019

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

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM, but this needs a rebase

@ronag ronag force-pushed the http-outgoing-cork branch 3 times, most recently from 650eeab to c650a63 Compare November 3, 2019 22:27
lib/_http_outgoing.js Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@ronag
Copy link
Member Author

ronag commented Nov 4, 2019

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
Copy link
Member Author

ronag commented Nov 4, 2019

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

@addaleax
Copy link
Member

addaleax commented Nov 5, 2019

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

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

@ronag
Copy link
Member Author

ronag commented Nov 6, 2019

rebased

@ronag
Copy link
Member Author

ronag commented Nov 11, 2019

@Trott failed on compile V8?

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Nov 12, 2019

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
Copy link
Member Author

ronag commented Nov 12, 2019

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

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Nov 12, 2019

@nodejs/http This could use more reviews.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

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
PR-URL: #29053
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Nov 13, 2019
PR-URL: #29053
Reviewed-By: Anna Henningsen <[email protected]>
@addaleax
Copy link
Member

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

@addaleax addaleax closed this Nov 13, 2019
MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
PR-URL: #29053
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
PR-URL: #29053
Reviewed-By: Anna Henningsen <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Nov 19, 2019
@targos 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
PR-URL: #29053
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this pull request Jan 13, 2020
PR-URL: #29053
Reviewed-By: Anna Henningsen <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
PR-URL: #29053
Reviewed-By: Anna Henningsen <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
PR-URL: #29053
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

7 participants