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: fix tlsSocket.setMaxSendFragment results in an abort #38170

Merged
merged 1 commit into from
Apr 21, 2021
Merged

tls: fix tlsSocket.setMaxSendFragment results in an abort #38170

merged 1 commit into from
Apr 21, 2021

Conversation

EladKeyshawn
Copy link

@EladKeyshawn EladKeyshawn commented Apr 9, 2021 •
edited by aduh95
Loading

Fixes: #38169

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. tls Issues and PRs related to the tls subsystem. labels Apr 9, 2021
aduh95
aduh95 reviewed Apr 9, 2021
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Can you please add tests?

lib/_tls_wrap.js Outdated Show resolved Hide resolved
@EladKeyshawn
Copy link
Author

Can you please add tests?
On it :)

aduh95
aduh95 reviewed Apr 9, 2021
View reviewed changes
lib/_tls_wrap.js Outdated Show resolved Hide resolved
lib/_tls_wrap.js Outdated Show resolved Hide resolved
test/parallel/test-tls-max-send-fragment.js Outdated Show resolved Hide resolved
test/parallel/test-tls-max-send-fragment.js Outdated Show resolved Hide resolved
aduh95
aduh95 reviewed Apr 10, 2021
View reviewed changes
test/parallel/test-tls-max-send-fragment.js Outdated Show resolved Hide resolved
test/parallel/test-tls-max-send-fragment.js Outdated Show resolved Hide resolved
test/parallel/test-tls-max-send-fragment.js Outdated Show resolved Hide resolved
test/parallel/test-tls-max-send-fragment.js Outdated Show resolved Hide resolved
test/parallel/test-tls-max-send-fragment.js Outdated Show resolved Hide resolved
test/parallel/test-tls-max-send-fragment.js Outdated Show resolved Hide resolved
@BridgeAR
Copy link
Member

@EladKeyshawn thanks a lot for addressing these issues! Since you open a couple PRs at the moment, would you mind reading about our commit message guidelines? They are referenced when opening a PR as well :-)

benjamingr
benjamingr approved these changes Apr 10, 2021
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Thanks 🙏

@benjamingr
Copy link
Member

Please consider addressing Antoine's nits and taking Ruben's advice regarding the collaborator guidelines :]

@EladKeyshawn
Copy link
Author

Please consider addressing Antoine's nits and taking Ruben's advice regarding the collaborator guidelines :]

Thanks man!

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@EladKeyshawn
Copy link
Author

Can someone help me understand why the CI won't pass?

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@EladKeyshawn EladKeyshawn changed the title Suggest fix - tlsSocket.setMaxSendFragment results in an abort tls: fix tlsSocket.setMaxSendFragment results in an abort Apr 15, 2021
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 18, 2021 •
edited by RaisinTen
Loading

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

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 18, 2021
@EladKeyshawn
Copy link
Author

@benjamingr Hey is this waiting for something?

@benjamingr
Copy link
Member

@ronag I think this can land :)

@Ayase-252 Ayase-252 mentioned this pull request Apr 21, 2021
Closed
@eladkishon @aduh95
tls: fix tlsSocket.setMaxSendFragment abort
6e3f985 
PR-URL: nodejs#38170
Fixes: nodejs#38169
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@aduh95 aduh95 force-pushed the eladkeyshawn-tls-setMaxSendFragment-abort branch from 6a269bd to 6e3f985 Compare April 21, 2021 21:18
@aduh95
Copy link
Contributor

aduh95 commented Apr 21, 2021

Landed in 6e3f985

@aduh95 aduh95 merged commit 6e3f985 into nodejs:master Apr 21, 2021
@EladKeyshawn EladKeyshawn deleted the eladkeyshawn-tls-setMaxSendFragment-abort branch April 22, 2021 06:08
targos pushed a commit that referenced this pull request Apr 29, 2021
@eladkishon @targos
tls: fix tlsSocket.setMaxSendFragment abort
f6745e9 
PR-URL: #38170
Fixes: #38169
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request May 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 left review comments

@jasnell jasnell jasnell approved these changes

@benjamingr benjamingr benjamingr approved these changes

@lpinca lpinca lpinca 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. needs-ci PRs that need a full CI run. 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.

tlsSocket.setMaxSendFragment results in an abort
10 participants
@EladKeyshawn @BridgeAR @benjamingr @nodejs-github-bot @aduh95 @jasnell @lpinca @targos @RaisinTen @eladkishon