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

net: handle socket.write(cb) edge case by santigimeno · Pull Request #45922 · nodejs/node · GitHub

/ node Public

net: handle socket.write(cb) edge case - #45922

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
santigimeno:santi/fix_30841
Jan 1, 2023
ConversationCommits (1)ChecksFiles changed
Merged

net: handle socket.write(cb) edge case#45922
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
santigimeno:santi/fix_30841

Conversation

santigimeno commented Dec 20, 2022

Copy link
Copy Markdown
Member

Make sure that when calling write() on a connecting socket, the callback is called if the socket is destroyed before the connection is established.

Fixes: #30841

nodejs-github-bot commented Dec 20, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/net

nodejs-github-bot added needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem. labels Dec 20, 2022
lpinca reviewed Dec 20, 2022
View reviewed changes
Comment thread test/parallel/test-net-write-cb-on-destroy.js Outdated

lpinca Dec 20, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
socket.on('connect', common.mustNotCall());

Comment thread test/parallel/test-net-write-cb-on-destroy.js Outdated

lpinca Dec 20, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think this error is a bit misleading. The socket is not closed when socket.write() is called. I think an error like "The socket was closed before the connection was established" would be be better. Anyway this is not a blocker.

Comment thread test/parallel/test-net-write-cb-on-destroy.js Outdated

lpinca Dec 20, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you append -before-connect to the test file name?

mcollina approved these changes Dec 21, 2022
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

santigimeno commented Dec 26, 2022

Copy link
Copy Markdown
Member Author

@lpinca I think I addressed your comments. PTAL. Thanks

lpinca approved these changes Dec 27, 2022
View reviewed changes
Comment thread lib/internal/errors.js Outdated
net: handle socket.write(cb) edge case
62ef1e5 
Make sure that when calling `write()` on a connecting socket, the
callback is called if the socket is destroyed before the connection is
established.

Fixes: nodejs#30841
santigimeno added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 27, 2022
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 27, 2022

nodejs-github-bot commented Dec 27, 2022

Copy link
Copy Markdown
Collaborator

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

mcollina approved these changes Dec 27, 2022
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

mcollina added commit-queue Add this label to land a pull request using GitHub Actions. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed needs-ci PRs that need a full CI run. labels Jan 1, 2023
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 1, 2023
nodejs-github-bot merged commit a03529d into nodejs:main Jan 1, 2023

nodejs-github-bot commented Jan 1, 2023

Copy link
Copy Markdown
Collaborator

Landed in a03529d

RafaelGSS pushed a commit that referenced this pull request Jan 2, 2023
net: handle socket.write(cb) edge case
b58a604 
Make sure that when calling `write()` on a connecting socket, the
callback is called if the socket is destroyed before the connection is
established.

Fixes: #30841
PR-URL: #45922
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
RafaelGSS mentioned this pull request Jan 2, 2023
Merged
RafaelGSS pushed a commit that referenced this pull request Jan 4, 2023
net: handle socket.write(cb) edge case
715e428 
Make sure that when calling `write()` on a connecting socket, the
callback is called if the socket is destroyed before the connection is
established.

Fixes: #30841
PR-URL: #45922
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Jan 5, 2023
net: handle socket.write(cb) edge case
4d62b09 
Make sure that when calling `write()` on a connecting socket, the
callback is called if the socket is destroyed before the connection is
established.

Fixes: #30841
PR-URL: #45922
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
net: handle socket.write(cb) edge case
f6cd0dc 
Make sure that when calling `write()` on a connecting socket, the
callback is called if the socket is destroyed before the connection is
established.

Fixes: #30841
PR-URL: #45922
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
juanarbol mentioned this pull request Jan 28, 2023
Merged
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
net: handle socket.write(cb) edge case
3ad584c 
Make sure that when calling `write()` on a connecting socket, the
callback is called if the socket is destroyed before the connection is
established.

Fixes: #30841
PR-URL: #45922
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
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

mcollina mcollina approved these changes
ShogunPanda ShogunPanda approved these changes
addaleax addaleax approved these changes
lpinca lpinca approved these changes
cjihrig cjihrig approved these changes
JungMinu JungMinu approved these changes

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

net: write(cb) not called if destroy():ed before 'connect'

8 participants

Back | FazBrowse Home | New Git URL