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

doc: mention errors thrown by methods called on an unbound dgram.Socket #33983

Closed
wants to merge 3 commits into from

Conversation

mkrawczuk
Copy link
Contributor

While working on this I have discovered an inconsistency: given dgram.Socket instance is undbound, calling address() on it results in a system error EBADF being thrown, while calling remoteAddress() results in ERR_SOCKET_DGRAM_NOT_CONNECTED.

Also, calling send() on an unbound socket throws ERR_SOCKET_BAD_PORT which might also be found misguiding.

These I believe are easy fixes. Once the documentation resembles the current state of things I can open another PR introducing a fix to it.

Also mentioned a side effect caused by socket.addMembership() and socket.addSourceSpecificMembership(). Not sure if it is desired.

@nodejs-github-bot nodejs-github-bot added dgram Issues and PRs related to the dgram subsystem / UDP. doc Issues and PRs related to the documentations. labels Jun 20, 2020
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

I agree, the error system is very inconsistent here, unfortunately … 😕 Thanks for the PR!

doc/api/dgram.md Outdated Show resolved Hide resolved
doc/api/dgram.md Outdated Show resolved Hide resolved
doc/api/dgram.md Outdated Show resolved Hide resolved
doc/api/dgram.md Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Jun 20, 2020

The text is fine as is, but if you want to fine-tune it even more, here's one more optional suggestion: Instead of it, I think this method would have been a little better? So "...this method will implicitly bind..." and "This method throws EWHATEVER..." and so forth.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 20, 2020
@mkrawczuk
Copy link
Contributor Author

Yeah, let's not be satisfied with 99% correct if we can be 100% correct with almost no effort.

Trott pushed a commit that referenced this pull request Jun 24, 2020
PR-URL: #33983
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member

Trott commented Jun 24, 2020

Landed in 91d9cdf

@Trott Trott closed this Jun 24, 2020
MylesBorins pushed a commit that referenced this pull request Jul 14, 2020
PR-URL: #33983
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 14, 2020
MylesBorins pushed a commit that referenced this pull request Jul 16, 2020
PR-URL: #33983
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
PR-URL: #33983
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. dgram Issues and PRs related to the dgram subsystem / UDP. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants