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: topic blocking vs non-blocking #5326

Closed
wants to merge 4 commits into from
Closed

Conversation

jrit
Copy link
Contributor

@jrit jrit commented Feb 19, 2016

The need for an overview of blocking vs non-blocking was
identified in the docs WG Q1 roadmap. As there are several
topics also pending creation, this one tries to hit the correct
level of detail based on completion of the others. One
which is referenced is
https://github.com/nodejs/node/pull/4936/files and URLs
within this PR need to change based on where that will land
on the node website.

Any reviews appreciated, thank you.

@chrisdickinson @techjeffharris

The need for an overview of blocking vs non-blocking was
identified in the docs WG Q1 roadmap. As there are several
topics also pending creation, this one tries to hit the correct
level of detail based on completion of the others.  One
which is referenced is
https://github.com/nodejs/node/pull/4936/files and URLs
within this PR need to change based on where that will land
on the node website.
@mscdex mscdex added the doc Issues and PRs related to the documentations. label Feb 19, 2016
## Blocking

Blocking is when the execution of additional JavaScript in the Node.js process
must wait until an I/O operation completes. Blocking may occur when using any of
Copy link
Member

Choose a reason for hiding this comment

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

nit: it's not just limited to i/o operations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be more appropriate to just take I/O out of the sentence or is there a better way to explain this? My goal is to distinguish that the process is waiting not just because JavaScript is executing.

Copy link
Member

Choose a reason for hiding this comment

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

Leaving the I/O in there is fine since that's the most likely case. It may just be worthwhile indicating that non-I/O operations can cause it to bog down as well. crypto operations, for instance.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure you consider things like waiting for synchronization ( a lock for instance) I/O.

@jasnell
Copy link
Member

jasnell commented Feb 19, 2016

generally LGTM. /cc @nodejs/documentation

```

The above places a non-blocking call to `unlink` within the callback of
`readFile` which guarantees the correct order of operations.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you refer here always to fs.unlink(), fs.readFile() for consistency?

@eljefedelrodeodeljefe
Copy link
Contributor

minus nits LGTM

also implement blocking methods, which usually have names that end with `Sync`.


## Comparing Code

Choose a reason for hiding this comment

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

Could we highlight: put this in Bold this is a synchronous file read -> this is a synchronous file read
then highlight: equivalent asynchronous example to equivalent asynchronous example
I think it's easier to notice.

@Knighton910
Copy link

🐢 LGTM

@Qard
Copy link
Member

Qard commented Feb 20, 2016

LGTM too, minus the nits.

Blocking is when the execution of additional JavaScript in the Node.js process
must wait until an I/O operation completes. Blocking may occur when using any of
the synchronous I/O methods in the Node.js standard library that use libuv. Use
of blocking methods prevents the event loop from doing additional work while
Copy link
Member

Choose a reason for hiding this comment

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

technically any native module can do any sort of blocking - it doesn't have to be through libuv but I guess that absolute correctness isn't a goal here.

provides clarification based on PR feedback
@jrit
Copy link
Contributor Author

jrit commented Feb 21, 2016

Thank you for the feedback everyone. Aside from specific formatting, the biggest issue seemed to be referring to I/O in a way that wasn't technically accurate. I tried to address that by rewriting some of the ## Blocking section and referring more directly to "non-JavaScript" instead of "I/O". I hope this will read more accurately without becoming unnecessarily technical.

prior knowledge of those topics is required. Readers are assumed to have a
basic understanding of the JavaScript language and Node.js callback pattern.

> "I/O" refers primarily to interaction with the system's disk and
Copy link
Contributor

Choose a reason for hiding this comment

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

This is quoting, right? Source is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a quote, just a note

The event loop is different than models in many other languages where additional
threads may be created to handle concurrent work. For an introduction to the
event loop see [Overview of the Event Loop, Timers, and
`process.nextTick()`](https://github.com/nodejs/node/pull/4936)
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, PRs are WIP items. So quoting them is a not a good idea I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This shouldn't be merged with this like this, my issue is that there should be a reference to the other topic on nodejs.org but the PR hasn't even been merged into master yet. This was mentioned above when the PR was opened.

additional clarifications
@techjeffharris
Copy link
Contributor

After one read LGTM, sans the link to #4936. I'll read through it again later to look for nits. Great Job!

@jasnell
Copy link
Member

jasnell commented Mar 2, 2016

Generally LGTM with the nits addressed.

@benjamingr
Copy link
Member

I'm +1 on merging and then improving it later - so LGTM.

@laosb
Copy link

laosb commented Mar 5, 2016

LGTM

@jrit
Copy link
Contributor Author

jrit commented Mar 17, 2016

Removed reference to an open PR which was blocking this, it makes more sense to add cross linking at a later date when relevant topics have been merged. Also removed reference to specific percent performance change that had objections from @jasnell and others

This should be in a good place to merge now. Thanks everyone for your feedback.

@jasnell
Copy link
Member

jasnell commented Mar 17, 2016

LGTM

@Qard
Copy link
Member

Qard commented Mar 17, 2016

LGTM too.

@benjamingr
Copy link
Member

Landed in 5f1eb43 - thanks!

@benjamingr benjamingr closed this Mar 19, 2016
benjamingr pushed a commit that referenced this pull request Mar 19, 2016
The need for an overview of blocking vs non-blocking was
identified in the docs WG Q1 roadmap. As there are several
topics also pending creation, this one tries to hit the correct
level of detail based on completion of the others.  One
which is referenced is
https://github.com/nodejs/node/pull/4936/files and URLs
within this PR need to change based on where that will land
on the node website.

PR-URL: #5326
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
The need for an overview of blocking vs non-blocking was
identified in the docs WG Q1 roadmap. As there are several
topics also pending creation, this one tries to hit the correct
level of detail based on completion of the others.  One
which is referenced is
https://github.com/nodejs/node/pull/4936/files and URLs
within this PR need to change based on where that will land
on the node website.

PR-URL: #5326
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
The need for an overview of blocking vs non-blocking was
identified in the docs WG Q1 roadmap. As there are several
topics also pending creation, this one tries to hit the correct
level of detail based on completion of the others.  One
which is referenced is
https://github.com/nodejs/node/pull/4936/files and URLs
within this PR need to change based on where that will land
on the node website.

PR-URL: #5326
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Fishrock123 pushed a commit that referenced this pull request Mar 22, 2016
The need for an overview of blocking vs non-blocking was
identified in the docs WG Q1 roadmap. As there are several
topics also pending creation, this one tries to hit the correct
level of detail based on completion of the others.  One
which is referenced is
https://github.com/nodejs/node/pull/4936/files and URLs
within this PR need to change based on where that will land
on the node website.

PR-URL: #5326
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.