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

tls: handle cases where the raw socket is destroyed #49980

Merged
merged 1 commit into from
Oct 4, 2023

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Sep 30, 2023

Ensure that the 'close' event is emitted on a TLSSocket when it is
created from an existing raw net.Socket and the raw socket is not
closed cleanly (destroyed).

Refs: 048e0bec5147
Refs: #49902 (comment)
Fixes: #49902

@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 Sep 30, 2023
@lpinca lpinca force-pushed the emit/close-event branch 2 times, most recently from 9c7a6df to c14baa7 Compare September 30, 2023 15:20
@lpinca
Copy link
Member Author

lpinca commented Sep 30, 2023

cc: @pimterry

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2023
@nodejs-github-bot
Copy link
Collaborator

Ensure that the `'close'` event is emitted on a `TLSSocket` when it is
created from an existing raw `net.Socket` and the raw socket is not
closed cleanly (destroyed).

Refs: nodejs@048e0bec5147
Refs: nodejs#49902 (comment)
Fixes: nodejs#49902
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2023
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.

Actual fix LGTM, I wonder if we shouldn't nextTick the callback in this case since I thhinkk it can be called synchronously in this case and I'm not sure it was possible before

That said, "maybe not 100% inconsistently timed" is still much better than "not sure if called at all"

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2023
@lpinca
Copy link
Member Author

lpinca commented Sep 30, 2023

Actual fix LGTM, I wonder if we shouldn't nextTick the callback in this case since I thhinkk it can be called synchronously in this case and I'm not sure it was possible before

I thought about that, but it is called after the 'close' event on the raw net.Socket so definitely not synchronously.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@pimterry pimterry left a comment

Choose a reason for hiding this comment

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

Good catch! Makes sense, LGTM 👍

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 4, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 4, 2023
@nodejs-github-bot nodejs-github-bot merged commit b1ada0a into nodejs:main Oct 4, 2023
27 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in b1ada0a

@lpinca lpinca deleted the emit/close-event branch October 4, 2023 14:24
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
Ensure that the `'close'` event is emitted on a `TLSSocket` when it is
created from an existing raw `net.Socket` and the raw socket is not
closed cleanly (destroyed).

Refs: nodejs@048e0bec5147
Refs: nodejs#49902 (comment)
Fixes: nodejs#49902
PR-URL: nodejs#49980
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request Nov 11, 2023
Ensure that the `'close'` event is emitted on a `TLSSocket` when it is
created from an existing raw `net.Socket` and the raw socket is not
closed cleanly (destroyed).

Refs: 048e0bec5147
Refs: #49902 (comment)
Fixes: #49902
PR-URL: #49980
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024
Ensure that the `'close'` event is emitted on a `TLSSocket` when it is
created from an existing raw `net.Socket` and the raw socket is not
closed cleanly (destroyed).

Refs: nodejs@048e0bec5147
Refs: nodejs#49902 (comment)
Fixes: nodejs#49902
PR-URL: nodejs#49980
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

parallel.test-tls-socket-close is flaky
6 participants