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

Fixed socket leakage in TLS connection #3254

Merged
merged 2 commits into from Aug 13, 2021
Merged

Fixed socket leakage in TLS connection #3254

merged 2 commits into from Aug 13, 2021

Conversation

gugu
Copy link
Contributor

@gugu gugu commented Aug 12, 2021 •
edited

  1. Added check for writable state of the socket before trying to end request gracefully
  2. Node.js updated their official documentation, so I changed link to the issue with a link to their official documentation

Fixes #3251

Checklist

@gugu
Fixed socket leakage in TLS connection
bf3eb99 
1. Added check for writable state of the socket before trying to end request gracefully
2. Node.js updated their official documentation, so I changed link to the issue with a link to their official documentation

Fixes fastify#3251
@Eomm
Copy link
Member

Eomm commented Aug 12, 2021

ERROR: Coverage for branches (99.91%) does not meet global threshold (100%)

The coverage is decreased, so the CI is failing. COuld you arrange a test for this change?

@mcollina
Copy link
Member

Awesome finding! Do you know which error code is triggering this problem?

@gugu gugu marked this pull request as draft August 12, 2021 15:32
@gugu
Copy link
Contributor Author

gugu commented Aug 12, 2021

PR is incorrect, and I'm adding a test

@gugu gugu marked this pull request as ready for review August 12, 2021 19:41
@gugu
Copy link
Contributor Author

gugu commented Aug 12, 2021

done

delvedor
delvedor approved these changes Aug 12, 2021
View reviewed changes
Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

Great catch!!

@Eomm Eomm added the bugfix Issue or PR that should land as semver patch label Aug 13, 2021
mcollina
mcollina approved these changes Aug 13, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit 3956e68 into fastify:main Aug 13, 2021
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mcollina mcollina mcollina approved these changes

@delvedor delvedor delvedor approved these changes

@Eomm Eomm Eomm approved these changes

Assignees
No one assigned
Labels
bugfix Issue or PR that should land as semver patch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Socket leakage on tls handshake timeout in https protocol
4 participants
@gugu @Eomm @mcollina @delvedor