doc: clarify when `'listening'` event is emitted by dsinecos · Pull Request #23959 · nodejs/node

dsinecos · 2018-10-29T17:16:51Z

Clarify that the 'listening' event for a UDP socket in NodeJS is emitted when the
underlying OS socket is created.

This occurs when either socket.send() or socket.bind()
are first invoked for the respective socket.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

sam-github · 2018-10-29T17:44:11Z

doc/api/dgram.md

What do you mean by "addressable"? #23952 (comment) shows the socket not having an address until the send or bind.

@sam-github I meant that we have a socket object (returned by dgram.createSocket) on which we can attach listeners so its addressable in that sense. However Node.js creates the underlying OS socket only after send or bind are invoked. I did not mean addressable in the sense of the socket having an address.

Now I can see how using addressable introduces ambiguity.

I was thinking to rephrase as follows

"While the different socket methods can be invoked immediately after creation using dgram.createSocket, Node.js doesn't create the underlying OS socket until ..."

But I'm a little confused because some socket methods can be used such as attaching listeners, setTTL etc, others such as socket.address() would throw an error until the underlying OS socket is created.

Let me know if you have any suggestions on the rephrasing and if my understanding is correct.

Thanks :)

"addressable" means "has an address", and it doesn't yet have an address.

Its confusing in the docs for this API that there is an OS object called a "socket" and a Javascript class dgram.Socket that is also sometimes called a "socket". This problem pre-exists, but maybe you can avoid using the ambiguous "socket" terminology.

I would suggest something like:

The listening event is emitted once the dgram.Socket is addressable and can receive data. This happens either explicitly with socket.bind() or implicitly the first time data is sent using socket.send(). Until the dgram.Socket is listening the underlying system resources do not exist, and calls such as socket.address() and socket.SetTTL() will fail.

Also, it would be helpful for each method that cannot be called until the socket is listening to explicitly mention that in its documentation.

@sam-github I was thinking to remove the second para entirely (if the ambiguity cannot be removed) and leave just the first sentence which clarifies when the 'listening' event is emitted

"The 'listening' event is emitted on the first invocation of either socket.send()
or socket.bind() for the respective socket"

I don't know what you mean by "ambiguity". Your single sentence lacks information useful to the API user, information which was in my suggestion. If you don't want to do this, that's OK, I'll PR a follow up sometime to expand the docs.

@dsinecos In the interest of progress, how about we do that (remove the second paragraph), land this PR with just the one sentence changed, and then if you wish, you can open a second PR with this second paragraph added back in and it can be discussed/improved in that second PR?

@Trott That sounds good. Allow me a couple of days. I'll make the suggested changes and update the PR. Thanks :)

vsemozhetbyt · 2018-10-29T21:43:32Z

doc/api/dgram.md

Nit:

Suggested change

`dgram.createSocket()` NodeJS doesn't create the underlying OS socket until the

`dgram.createSocket()` Node.js doesn't create the underlying OS socket until the

Got it. I'll update this :)

Trott · 2018-11-20T23:32:18Z

@dsinecos Still working on this?

dsinecos · 2018-11-21T12:20:18Z

@Trott On the point that @sam-github raised, I have posted some alternatives I could think of.

I'd like some comments on that to move forward

sam-github · 2018-11-21T12:39:59Z

It looks like the last comments were mine, and there are no changes or comments since then. I think this is waiting on @dsinecos to move forward.

dsinecos · 2018-11-21T12:49:58Z

@sam-github I've commented with some alternatives based on the query you raised. Let me know your thoughts on that. I'll update the PR accordingly

Clarify that the `'listening'` event is emitted when the underlying OS socket is created This occurs when either `socket.send()` or `socket.bind()` are first invoked for the respective socket. Fixes: nodejs#23952

dsinecos · 2018-11-30T04:52:54Z

The build says it's failing on the commit message guidelines. I'm not able to notice the error I've made in the commit message. Please suggest :)

Trott · 2018-11-30T19:38:08Z

The build says it's failing on the commit message guidelines. I'm not able to notice the error I've made in the commit message. Please suggest :)

Commit message is probably fine. GitHub API is throttling us. (I believe there is a PR somewhere to work around the GitHub API limitations.)

Trott · 2018-11-30T19:38:38Z

/ping @sam-github LGTY?

lundibundi · 2019-11-24T14:00:45Z

doc/api/dgram.md

-The `'listening'` event is emitted whenever a socket begins listening for
-datagram messages. This occurs as soon as UDP sockets are created.
+The `'listening'` event is emitted on the first invocation of either `socket.send()`
+or `socket.bind()` for the respective socket.


Would this be clearer?

The `'listening'` event is emitted whenever a socket begins listening for datagram messages. This corresponds to the first invocation of either `socket.send()` or `socket.bind()` for the respective socket.

lundibundi · 2019-11-24T14:01:45Z

ping @sam-github @dsinecos

sam-github · 2018-10-30T15:27:31Z

doc/api/dgram.md

"addressable" means "has an address", and it doesn't yet have an address.

Its confusing in the docs for this API that there is an OS object called a "socket" and a Javascript class dgram.Socket that is also sometimes called a "socket". This problem pre-exists, but maybe you can avoid using the ambiguous "socket" terminology.

I would suggest something like:

The listening event is emitted once the dgram.Socket is addressable and can receive data. This happens either explicitly with socket.bind() or implicitly the first time data is sent using socket.send(). Until the dgram.Socket is listening the underlying system resources do not exist, and calls such as socket.address() and socket.SetTTL() will fail.

Also, it would be helpful for each method that cannot be called until the socket is listening to explicitly mention that in its documentation.

sam-github · 2018-11-02T15:46:27Z

doc/api/dgram.md

I don't know what you mean by "ambiguity". Your single sentence lacks information useful to the API user, information which was in my suggestion. If you don't want to do this, that's OK, I'll PR a follow up sometime to expand the docs.

BridgeAR · 2019-12-20T01:39:29Z

@dsinecos I am very sorry that this PR seems to have been overlooked at some point. It is some times difficult to keep up with the amount of changes.

Would you still be interesting in following up on the comments?

dsinecos · 2019-12-20T10:32:57Z

@Trott @sam-github @BridgeAR @lundibundi I'm sorry I missed the last few updates here. I'll not be able to work on this PR at the moment and cannot say for sure when I'll be able to pick it back up. Let me know if I should close it.

HarshithaKP · 2020-03-31T06:42:11Z

@dsinecos, Will you be able to progress on this PR now ? If not I would like to pick this up.

dsinecos · 2020-03-31T06:54:24Z

@HarshithaKP Please go ahead.

HarshithaKP · 2020-04-08T04:04:20Z

This can be closed as the intention is fulfilled in #32581.

nodejs-github-bot added dgram Issues and PRs related to the dgram subsystem / UDP. doc Issues and PRs related to the documentations. labels Oct 29, 2018

dsinecos mentioned this pull request Oct 29, 2018

UDP/Datagram Sockets documentation #23952

Closed

sam-github reviewed Oct 29, 2018

View reviewed changes

vsemozhetbyt reviewed Oct 29, 2018

View reviewed changes

doc: clarify when 'listening' event is emitted

70b0e8e

Clarify that the `'listening'` event is emitted when the underlying OS socket is created This occurs when either `socket.send()` or `socket.bind()` are first invoked for the respective socket. Fixes: nodejs#23952

dsinecos force-pushed the docs-update-dgram branch from a803b2a to 70b0e8e Compare November 29, 2018 11:12

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:52

lundibundi reviewed Nov 24, 2019

View reviewed changes

sam-github approved these changes Nov 25, 2019

View reviewed changes

devnexen force-pushed the master branch from e8a4568 to 5289f80 Compare December 26, 2019 19:46

HarshithaKP mentioned this pull request Mar 31, 2020

doc: clarify listening event #32581

Closed

4 tasks

Trott closed this Apr 8, 2020

	`dgram.createSocket()` NodeJS doesn't create the underlying OS socket until the
	`dgram.createSocket()` Node.js doesn't create the underlying OS socket until the

Uh oh!

Conversation

dsinecos commented Oct 29, 2018

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsinecos Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsinecos Nov 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Nov 20, 2018

Uh oh!

dsinecos commented Nov 21, 2018

Uh oh!

sam-github commented Nov 21, 2018

Uh oh!

dsinecos commented Nov 21, 2018

Uh oh!

dsinecos commented Nov 30, 2018

Uh oh!

Trott commented Nov 30, 2018

Uh oh!

Trott commented Nov 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lundibundi commented Nov 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Dec 20, 2019

Uh oh!

dsinecos commented Dec 20, 2019

Uh oh!

HarshithaKP commented Mar 31, 2020

Uh oh!

dsinecos commented Mar 31, 2020

Uh oh!

HarshithaKP commented Apr 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

dsinecos Oct 30, 2018 •

edited

Loading

dsinecos Nov 2, 2018 •

edited

Loading