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.

Sign up for GitHub

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

Jump to bottom

tls: connect supports highWaterMark option #32786

Closed
wants to merge 1 commit into from
Closed

tls: connect supports highWaterMark option #32786

wants to merge 1 commit into from

Conversation

rickyes
Copy link
Contributor

@rickyes rickyes commented Apr 11, 2020 •
edited
Loading

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 nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Apr 11, 2020
@addaleax
Copy link
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.

@rickyes
Copy link
Contributor Author

rickyes commented Apr 11, 2020

@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 rickyes changed the title lib: support https highWaterMark tls: support highWaterMark Apr 12, 2020
@rickyes rickyes changed the title tls: support highWaterMark tls: connect supports highWaterMark option Apr 12, 2020
@rickyes
Copy link
Contributor Author

rickyes commented Apr 12, 2020

@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 rickyes requested a review from addaleax April 13, 2020 01:42
@rickyes
Copy link
Contributor Author

rickyes commented Apr 13, 2020 •
edited
Loading

/cc @nodejs/crypto @jasnell

@himself65 himself65 added the review wanted PRs that need reviews. label Apr 13, 2020
himself65
himself65 requested changes Apr 13, 2020
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
doc/api/https.md Outdated Show resolved Hide resolved
@rickyes rickyes requested a review from himself65 April 13, 2020 07:56
addaleax
addaleax approved these changes Apr 13, 2020
View reviewed changes
doc/api/https.md Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
test/parallel/test-tls-connect-hwm-option.js Outdated Show resolved Hide resolved
@rickyes
Copy link
Contributor Author

rickyes commented Apr 25, 2020

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

@nodejs-github-bot
Copy link
Collaborator

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

@rickyes
Copy link
Contributor Author

rickyes commented Apr 26, 2020

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

I think it's ready.

@ronag ronag added author ready PRs that have at least one approval, no pending requests for changes, 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
@rickyes @puzpuzpuz
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
Copy link
Member

Landed in 58682d8

@puzpuzpuz puzpuzpuz closed this Apr 27, 2020
@rickyes rickyes deleted the https-highWaterMark branch April 27, 2020 14:24
BethGriggs pushed a commit that referenced this pull request Apr 27, 2020
@rickyes @BethGriggs
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 BethGriggs mentioned this pull request Apr 27, 2020
Merged
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
@rickyes @BridgeAR
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
@rickyes @BridgeAR
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 BridgeAR mentioned this pull request Apr 28, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Apr 28, 2020
@rickyes @BethGriggs
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 richardlau mentioned this pull request May 13, 2020
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@himself65 himself65 himself65 approved these changes

@puzpuzpuz puzpuzpuz puzpuzpuz approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, 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
@rickyes @addaleax @nodejs-github-bot @puzpuzpuz @jasnell @himself65 @targos @ronag