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

net: make readyState to be closed if socket is not opened #38249

Closed

Conversation

Ayase-252
Copy link
Member

This PR sets Socket.readyState to closed, when socket is not opened (still pending but not connecting).

Currently, Socket.readyState is open before socket is connected to a peer. It can be somehow confusing for users.

Fixes: #38247

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem. labels Apr 15, 2021
doc/api/net.md Outdated
@@ -1125,6 +1129,8 @@ This property represents the state of the connection as a string.
* If the stream is readable and writable, it is `open`.
* If the stream is readable and not writable, it is `readOnly`.
* If the stream is not readable and writable, it is `writeOnly`.
* If the stream is not opened or is neither readable nor writable, it is
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should avoid using words like 'open' here because it's an actual readyState value and might be confusing?

Perhaps something like:

Suggested change
* If the stream is not opened or is neither readable nor writable, it is
* If the stream is not connecting, connected, or is otherwise neither readable nor writable, it is

doc/api/net.md Outdated
@@ -1125,6 +1129,8 @@ This property represents the state of the connection as a string.
* If the stream is readable and writable, it is `open`.
Copy link
Contributor

@mscdex mscdex Apr 15, 2021

Choose a reason for hiding this comment

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

It might be nice to change stream here to socket here and elsewhere in this property's description as it makes more sense IMO.

@ronag ronag requested a review from lpinca April 15, 2021 22:19
@ronag
Copy link
Member

ronag commented Apr 15, 2021

@lpinca how is it with websockets?

Co-authored-by: Brian White <mscdex@mscdex.net>
@lpinca
Copy link
Member

lpinca commented Apr 16, 2021

ws does not use socket.readyState. I don't know if other WebSocket libraries use it.

@addaleax addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 17, 2021
@addaleax
Copy link
Member

I think it would also be okay to document the current state of things.

@Ayase-252
Copy link
Member Author

If I understand correctly, it won't be able to land in current major version (v16 I assume), should we note the current state in the document in the first place? If so, I could open a new PR to amend the document.

@cctv1005s
Copy link

cctv1005s commented May 19, 2022

hi, for this pr, I have a question for why the readyState change from closed to open in node@v14.x ?

@Ayase-252

@aduh95
Copy link
Contributor

aduh95 commented Sep 19, 2023

This would need a rebase if we still want to land it.

@aduh95 aduh95 added the stalled Issues and PRs that are stalled. label Sep 19, 2023
@github-actions
Copy link
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

Copy link
Contributor

Closing this because it has stalled. Feel free to reopen if this issue/PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.

@github-actions github-actions bot closed this Nov 13, 2023
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. net Issues and PRs related to the net subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Socket.readyState is initially open
8 participants