doc: clarify lifecycle of domain sockets #19471

gireeshpunathil · 2018-03-20T09:19:18Z

In UNIX, the domain sockets once created persists until unlinked.
Clarify which ones are persisted and which ones are cleared manually.

In Windows, named pipes are cleared based on reference count,
implemented by the underlying system. Disambiguate this from
Garbage collection of the Node.js runtime.

Refs: nodejs/help#1080

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

vsemozhetbyt · 2018-03-20T10:07:13Z

doc/api/net.md

+on file creation. If the UNIX domain socket (that is visible as a file system
+path) is created and used in conjunction with one of Node.js' API abstractions
+such as [`net.createServer()`][], it will be unlinked as part of
+[`server.close()`][]. On the other hand, if it is created and used otuside of


Nit: otuside -> outside

thanks @vsemozhetbyt - fixed.

Trott · 2018-03-20T13:06:49Z

doc/api/net.md

-persist*, they are removed when the last reference to them is closed. Do not
-forget JavaScript string escaping requires paths to be specified with
-double-backslashes, such as:
+persist*, they are removed when the last reference to them is closed - managed


Hmm...So before you edited it, this sentence was already a run-on sentence due to the comma splice. I'm not sure I understand the material your adding in this paragraph. Does this seem right?:

Pipes will *not persist*. They are removed when the last reference to them is closed. Unlike UNIX domain sockets, Windows will close and remove the pipe when the process exits. JavaScript string escaping requires paths to be specified with extra backslash escaping such as:

thanks @Trott - addressed, incorporated your text as it is with only a single word addition.

Trott · 2018-03-20T13:07:44Z

Nit: While we're editing these paragraphs, maybe fix path name -> pathname (consistent with other docs) and name space -> namespace (also consistent with other docs)?

Trott · 2018-03-20T13:08:18Z

@nodejs/documentation

benjamingr · 2018-03-20T16:54:27Z

doc/api/net.md

-persist*, they are removed when the last reference to them is closed. Do not
-forget JavaScript string escaping requires paths to be specified with
-double-backslashes, such as:
+sequences. Despite appearances, the pipe namespace is flat. Pipes will 


Despite how it might look or despite appearance?

"Despite appearances" is idiomatic US English. Don't know that I've ever heard "Despite appearance" and it would make me think the writer forgot the s at the end of appearance. "Despite how it might look" is a fine alternative.

@benjamingr @Trott - addressed, thanks.

benjamingr · 2018-03-20T16:55:22Z

doc/api/net.md

-on file creation. It will be visible in the filesystem, and will *persist until
-unlinked*.
+on file creation. If the UNIX domain socket (that is visible as a file system
+path) is created and used in conjunction with one of Node.js' API abstractions


I'm not sure this is clear to be honest, I think an example would help more.

@benjamingr - Can you please clarify what is not clear? and example for what?

ping @benjamingr to see what can be done.

@gireeshpunathil I'm not blocking - I'm just not sure the description here is less confusing than before. I don't have any ideas as to what I'd write differently - sorry!

@benjamingr - thanks, sure. I have asked the OP ( @boneskull ) to have a look too to see if any improvements can be made. Else I will move towards landing, thanks!

In UNIX, the domain sockets once created persists until unlinked. Clarify which ones are persisted and which ones are cleared manually. In Windows, named pipes are cleared based on reference count, implemented by the underlying system. Disambiguate this from Garbage collection of the Node.js runtime. Refs: nodejs/help#1080

gireeshpunathil · 2018-03-29T08:09:36Z

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite/356/

In UNIX, the domain sockets once created persists until unlinked. Clarify which ones are persisted and which ones are cleared manually. In Windows, named pipes are cleared based on reference count, implemented by the underlying system. Disambiguate this from Garbage collection of the Node.js runtime. Refs: nodejs/help#1080 PR-URL: #19471 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>

gireeshpunathil · 2018-04-06T14:22:50Z

the PR is landed, but I am wondering whether something went wrong or not - it had a merge conflict that I manually resolved by following the merge conflict resolve sequence through the command line. everything looks reasonable, except that it produced 2 more commits. Are we good here? @Trott / @joyeecheung

lpinca · 2018-04-06T14:26:19Z

I think it's too late for a force push. The allowed window is 10 min.

gireeshpunathil · 2018-04-06T14:29:00Z

@lpinca - but what went wrong? what needs to be reverted now?

lpinca · 2018-04-06T14:30:39Z

I honestly don't know.

what needs to be reverted now?

Nothing, I guess.

gireeshpunathil · 2018-04-06T14:31:14Z

thanks, good to know!

Trott · 2018-04-06T16:29:33Z

I think we should force push out those merge commits, but they've been there for two hours so I'd like another opinion. @nodejs/release @nodejs/tsc

MylesBorins · 2018-04-06T16:35:22Z

I'd say +1... but would want more TSC people to chime in

evanlucas · 2018-04-06T16:50:34Z

+1 from me

addaleax · 2018-04-06T16:54:40Z

@gireeshpunathil this would need to be re-landed now... re-opening the PR doesn't work, unfortunately.

(But if there are conflicts, maybe open another PR anyway?)

Trott · 2018-04-06T17:00:07Z

I'm re-landing now. There don't appear to be any conflicts...

Trott · 2018-04-06T17:03:00Z

make test-doc is failing on master. 😞

lpinca · 2018-04-06T17:05:46Z

Unrelated question: should we increase the allowed force push window to something bigger than 10 min? @gireeshpunathil noticed the issue ~18 min after landing and I suggested to not force push as 10 min had already passed.

Trott · 2018-04-06T17:09:58Z

Ignore me: make test-doc is not failing. Landing!

In UNIX, the domain sockets once created persists until unlinked. Clarify which ones are persisted and which ones are cleared manually. In Windows, named pipes are cleared based on reference count, implemented by the underlying system. Disambiguate this from Garbage collection of the Node.js runtime. Refs: nodejs/help#1080 PR-URL: nodejs#19471 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>

Trott · 2018-04-06T17:10:47Z

Landed in 53aaa55

addaleax · 2018-04-06T17:13:58Z

@lpinca I still think 10 min is a good interval for the more common types of problems, that can be fixed by sending a quick fix-up PR or commenting metadata under the commits or something similar

Maybe we could document the option of pinging the TSC explicitly? (Like, we can't 100 % outlaw force-pushes after more than 10 minutes anyway -- it's always going to be possible that we have to amend or remove commits for legal reasons.)

lpinca · 2018-04-06T17:16:37Z

Maybe something slightly bigger (15 min?) is more appropriate? GitHub notifications are fast but not instant and not all collaborators are on IRC.

Trott · 2018-04-06T18:41:46Z

I'm not strictly opposed to increasing the time, but two things to consider:

I don't think a bigger window would have helped here. Basically, the 10-minute window is "oops, I better fix this"! Things that you know you messed up and want to fix quickly. Anything that requires a bit of conversation/coordination is likely to have to be escalated to TSC anyway. A smaller window means it gets escalated faster, which is a good thing.
Changing it means that some people will think it's 10 minutes and other people will think it's whatever it gets changed to, because not everybody gets the memo when we make a rule change. Therefore, we should be very judicious in changing rules/guidelines. (I mean, it's not the end of the world if someone doesn't know they have 15 minutes instead of 10, but just as a general principle, we probably shouldn't make rule changes that aren't high-value.)

lpinca · 2018-04-06T19:26:24Z

I don't think a bigger window would have helped here.

In this particular case If it was a little bigger I would have suggested to @gireeshpunathil to force push but it was almost twice the allowed time.

I agree with the other points though.

gireeshpunathil · 2018-04-07T02:23:10Z

Apologies for causing the mess. I take the blame for not fully understanding the process, and promise to be better next time. Thanks to the team those who stepped in and acted instantly to clean it!

Looking back at what has happened:

The PR lands through regular proceedings, the file (doc/api/net.md) did not show any conflict in the web interface
After the git push, I notice the error message relating to 'merge conflict' on the file, in the web ui.
It (the web ui) offered a 'solution' with a button - 'resolve merge conflicts automatically' or similar.
I recollected the first point in collaborator guide Never use GitHub's green "Merge Pull Request" button. but then mistook it to be the problem only if you use it through the UI. I followed its command line equivalent! , assuming that sequence will address the drawbacks of the web ui method.
I expected the command line alternative to do something similar to what git squash does.
Immediately after the git push I noticed the 2 extra commits, but was not sure whether it was so harmful!

Would increasing force push threshold have helped me in this case?

No. As my perception about the problem and idea of solving it was not coherent, 5 or 10 extra minutes would not make any difference.

However, IMO 10 minutes can be too less for someone (like me) to realize the issue, figure out what to do and apply the changes - of course it depends on the experience level of who lands PRs.

document the option of pinging the TSC explicitly?

+1 - yes, that will really help!

In UNIX, the domain sockets once created persists until unlinked. Clarify which ones are persisted and which ones are cleared manually. In Windows, named pipes are cleared based on reference count, implemented by the underlying system. Disambiguate this from Garbage collection of the Node.js runtime. Refs: nodejs/help#1080 PR-URL: #19471 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>

nodejs-github-bot added doc Issues and PRs related to the documentations. net Issues and PRs related to the net subsystem. labels Mar 20, 2018

lpinca approved these changes Mar 20, 2018

View reviewed changes

vsemozhetbyt reviewed Mar 20, 2018

View reviewed changes

vsemozhetbyt approved these changes Mar 20, 2018

View reviewed changes

vsemozhetbyt added the fast-track PRs that do not need to wait for 48 hours to land. label Mar 20, 2018

gireeshpunathil force-pushed the pipe_doc branch from 2dc3895 to 50477df Compare March 20, 2018 10:38

Trott reviewed Mar 20, 2018

View reviewed changes

gireeshpunathil force-pushed the pipe_doc branch from 7d048d3 to 430db5c Compare March 20, 2018 13:27

benjamingr reviewed Mar 20, 2018

View reviewed changes

gireeshpunathil force-pushed the pipe_doc branch from e4894e6 to 8ca426c Compare March 21, 2018 07:33

gireeshpunathil mentioned this pull request Apr 2, 2018

net: Don't unlink unix socket on server.close() #19729

Open

gireeshpunathil added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 6, 2018

gireeshpunathil merged commit 8ca426c into nodejs:master Apr 6, 2018

vsemozhetbyt mentioned this pull request Apr 6, 2018

Merge commits on master #19854

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: clarify lifecycle of domain sockets #19471

doc: clarify lifecycle of domain sockets #19471

gireeshpunathil commented Mar 20, 2018

vsemozhetbyt Mar 20, 2018

gireeshpunathil Mar 20, 2018

Trott Mar 20, 2018

gireeshpunathil Mar 20, 2018

Trott commented Mar 20, 2018

Trott commented Mar 20, 2018

benjamingr Mar 20, 2018

Trott Mar 20, 2018

gireeshpunathil Mar 21, 2018

benjamingr Mar 20, 2018

gireeshpunathil Mar 21, 2018

gireeshpunathil Mar 29, 2018

benjamingr Mar 29, 2018

gireeshpunathil Mar 29, 2018

gireeshpunathil commented Mar 29, 2018

gireeshpunathil commented Apr 6, 2018

lpinca commented Apr 6, 2018

gireeshpunathil commented Apr 6, 2018

lpinca commented Apr 6, 2018

gireeshpunathil commented Apr 6, 2018

Trott commented Apr 6, 2018

MylesBorins commented Apr 6, 2018

evanlucas commented Apr 6, 2018

addaleax commented Apr 6, 2018

Trott commented Apr 6, 2018

Trott commented Apr 6, 2018

lpinca commented Apr 6, 2018 •

edited

Loading

Trott commented Apr 6, 2018

Trott commented Apr 6, 2018

addaleax commented Apr 6, 2018

lpinca commented Apr 6, 2018

Trott commented Apr 6, 2018

lpinca commented Apr 6, 2018

gireeshpunathil commented Apr 7, 2018

doc: clarify lifecycle of domain sockets #19471

doc: clarify lifecycle of domain sockets #19471

Conversation

gireeshpunathil commented Mar 20, 2018

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Mar 20, 2018

Trott commented Mar 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gireeshpunathil commented Mar 29, 2018

gireeshpunathil commented Apr 6, 2018

lpinca commented Apr 6, 2018

gireeshpunathil commented Apr 6, 2018

lpinca commented Apr 6, 2018

gireeshpunathil commented Apr 6, 2018

Trott commented Apr 6, 2018

MylesBorins commented Apr 6, 2018

evanlucas commented Apr 6, 2018

addaleax commented Apr 6, 2018

Trott commented Apr 6, 2018

Trott commented Apr 6, 2018

lpinca commented Apr 6, 2018 • edited Loading

Trott commented Apr 6, 2018

Trott commented Apr 6, 2018

addaleax commented Apr 6, 2018

lpinca commented Apr 6, 2018

Trott commented Apr 6, 2018

lpinca commented Apr 6, 2018

gireeshpunathil commented Apr 7, 2018

lpinca commented Apr 6, 2018 •

edited

Loading