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

tls: connect supports highWaterMark option by rickyes · Pull Request #32786 · nodejs/node · GitHub

/ node Public

tls: connect supports highWaterMark option - #32786

Closed
rickyes wants to merge 1 commit into
nodejs:masterfrom
rickyes:https-highWaterMark
ConversationCommits (1)ChecksFiles changed
Closed

tls: connect supports highWaterMark option#32786
rickyes wants to merge 1 commit into
nodejs:masterfrom
rickyes:https-highWaterMark

Conversation

rickyes commented Apr 11, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

fixes: #32781

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Apr 11, 2020

addaleax commented Apr 11, 2020

Copy link
Copy Markdown
Member

@rickyes A few things:

  • This is missing a documentation update, I think? I think both a change to the documentation and a changes: entry make sense here?
  • Does this really affect only HTTPS like the commit message implies? It looks like it affects all TLS connections? In that case, an additional test that doesn’t use HTTPS and only TLS seems like a good idea.
  • The commit message should probably start with tls:. Generally, lib: makes sense only for very generic changes or changes that do not affect the way that the public API behaves.

addaleax reviewed Apr 11, 2020
View reviewed changes
Comment thread test/parallel/test-https-hwm.js Outdated

rickyes commented Apr 11, 2020

Copy link
Copy Markdown
Contributor Author

@rickyes A few things:

  • This is missing a documentation update, I think? I think both a change to the documentation and a changes: entry make sense here?
  • Does this really affect only HTTPS like the commit message implies? It looks like it affects all TLS connections? In that case, an additional test that doesn’t use HTTPS and only TLS seems like a good idea.
  • The commit message should probably start with tls:. Generally, lib: makes sense only for very generic changes or changes that do not affect the way that the public API behaves.

Thanks for the reminder, I'll change it.

rickyes changed the title lib: support https highWaterMark tls: support highWaterMark Apr 12, 2020
rickyes changed the title tls: support highWaterMark tls: connect supports highWaterMark option Apr 12, 2020
rickyes force-pushed the https-highWaterMark branch from 254512b to 39e9cd4 Compare April 12, 2020 03:07

rickyes commented Apr 12, 2020

Copy link
Copy Markdown
Contributor Author

@rickyes A few things:

  • This is missing a documentation update, I think? I think both a change to the documentation and a changes: entry make sense here?
  • Does this really affect only HTTPS like the commit message implies? It looks like it affects all TLS connections? In that case, an additional test that doesn’t use HTTPS and only TLS seems like a good idea.
  • The commit message should probably start with tls:. Generally, lib: makes sense only for very generic changes or changes that do not affect the way that the public API behaves.

done

rickyes force-pushed the https-highWaterMark branch from 39e9cd4 to e6834c1 Compare April 12, 2020 03:21
rickyes requested a review from addaleax April 13, 2020 01:42

rickyes commented Apr 13, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

/cc @nodejs/crypto @jasnell

himself65 added the review wanted PRs that need reviews. label Apr 13, 2020
himself65 requested changes Apr 13, 2020
View reviewed changes
Comment thread doc/api/tls.md Outdated
Comment thread doc/api/https.md Outdated
himself65 reviewed Apr 13, 2020
View reviewed changes
Comment thread test/parallel/test-https-hwm.js Outdated
rickyes force-pushed the https-highWaterMark branch from e6834c1 to eb308da Compare April 13, 2020 07:43
rickyes requested a review from himself65 April 13, 2020 07:56
addaleax approved these changes Apr 13, 2020
View reviewed changes
Comment thread doc/api/https.md Outdated
Comment thread doc/api/tls.md Outdated
Comment thread test/parallel/test-tls-connect-hwm-option.js Outdated
rickyes force-pushed the https-highWaterMark branch from eb308da to e889a5b Compare April 13, 2020 13:07

rickyes commented Apr 25, 2020

Copy link
Copy Markdown
Contributor Author

Should be ready, can you help start a CI run ？ @addaleax @himself65

nodejs-github-bot commented Apr 25, 2020

Copy link
Copy Markdown
Collaborator

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

rickyes commented Apr 26, 2020

Copy link
Copy Markdown
Contributor Author

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

I think it's ready.

ronag added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed review wanted PRs that need reviews. labels Apr 26, 2020
puzpuzpuz pushed a commit that referenced this pull request Apr 27, 2020
tls: add highWaterMark option for connect
58682d8 
PR-URL: #32786
Fixes: #32781
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>

puzpuzpuz commented Apr 27, 2020

Copy link
Copy Markdown
Member

Landed in 58682d8

puzpuzpuz closed this Apr 27, 2020
rickyes deleted the https-highWaterMark branch April 27, 2020 14:24
BethGriggs pushed a commit that referenced this pull request Apr 27, 2020
tls: add highWaterMark option for connect
35fcfbe 
PR-URL: #32786
Fixes: #32781
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
BethGriggs mentioned this pull request Apr 27, 2020
Merged
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
tls: add highWaterMark option for connect
efb3c71 
PR-URL: #32786
Fixes: #32781
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
tls: add highWaterMark option for connect
f6274af 
PR-URL: #32786
Fixes: #32781
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
BridgeAR mentioned this pull request Apr 28, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Apr 28, 2020
tls: add highWaterMark option for connect
323da6f 
PR-URL: #32786
Fixes: #32781
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
richardlau mentioned this pull request May 13, 2020
Closed
4 tasks
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

jasnell jasnell approved these changes
addaleax addaleax approved these changes
himself65 himself65 approved these changes
+1 more reviewer
puzpuzpuz puzpuzpuz approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. tls Issues and PRs related to the tls subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

highWaterMark option is ignored in HTTPs request stream

8 participants

Back | FazBrowse Home | New Git URL