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

Put a warning against misusing *Sync methods to the API docs. #1684

Closed
ChALkeR opened this issue May 12, 2015 · 14 comments
Closed

Put a warning against misusing *Sync methods to the API docs. #1684

ChALkeR opened this issue May 12, 2015 · 14 comments
Labels
doc Issues and PRs related to the documentations.

Comments

@ChALkeR
Copy link
Member

ChALkeR commented May 12, 2015

Taking apart #1665.

While #1674 looks like a good thing, almost eveyone seems to think that it replaces #1665. But it doesn't touch the initial point of #1665 — to discourage using *Sync versions of the methods that could be async.

The reasoning is mostly the same as in #1665.

Two alternatives here:

  1. Create a separate page with a warning against improper usage of blocking methods and add a small but instantly visible reference from every *Sync method documentation to that page.
    Sample:

    fs.accessSync(path[, mode])

    Warning: this is blocking call, do not use in an asynchronous enviroment unless you know what you are doing. More info on some nice title (link).

    Synchronous version of fs.access. This throws if any accessibility checks fail, and does nothing otherwise.

  2. Create a separate page with a warning and move all *Sync methods documentation to that page, leaving only references (possibly with headers) to it on the corresponding module pages.
    Sample:

    fs.accessSync(path[, mode])

    A blocking call, the documentation is listed on some nice title (link).

Notice: I am not a native English speaker and can be a bit wrong with woridings.

@ChALkeR ChALkeR changed the title Put a warning against misusing *Sync methods in the API docs. Put a warning against misusing *Sync methods to the API docs. May 12, 2015
@mscdex mscdex added the doc Issues and PRs related to the documentations. label May 12, 2015
@benjamingr
Copy link
Member

I'd love it if io took a more active part in the documentation and education of its users about the paradigms it requires like most popular frameworks in other languages do.

This is similar to what MDN is doing but for cases where MDN simply does not apply - we shouldn't document Function.prototype.apply but it sure would be nice if we gave users a nicer introduction to the platform (vs. the language).

I think it would really help if we introduce a guide that explains this sort of stuff to new users - probably on another repo but with a direct link on the website. This can further be translated.

In addition, the official documentation should link there in Sync methods. We want to make it clear that these methods, while not deprecated have serious performance overhead for concurrent applications.

I think it's also best to accept that a lot of people use node/io.js today for things other than servers and applications. These users should not be discouraged from using node/io.js by the documentation.

@CrabDude
Copy link

it sure would be nice if we gave users a nicer introduction to the platform (vs. the language).

I think this is where the ecosystem is at evolutionarily. With 1.0, the major runtime concerns are largely addressed, and some manner of canonical education is the next layer beyond API enablement. It's encouraging to know that educational documentation PRs would be considered.

@Fishrock123
Copy link
Contributor

I'd be in favor of this if it was worded well.

@seishun
Copy link
Contributor

seishun commented Oct 16, 2015

Unfortunately, currently Node.js has two distinct groups of functions with "Sync" in them:

  1. Functions that block on I/O. They literally waste time and can take unpredictable amounts of time. It's almost always a bad idea to use them outside of the first tick. Examples: fs.readFileSync.
  2. Functions that happen to have an alternative that runs in a thread pool. Otherwise they work exactly the same as, for example, Buffer.concat. Time they take is proportional to input size and inversely proportional to CPU speed. Their usage is perfectly fine in most cases. Examples: zlib.deflateSync.

I've argued against lumping them together many times before (nodejs/node-v0.x-archive#7030, #5) but didn't get much attention, so this is what we have now. If we're going to warn against using Sync functions in the docs, then we need to clearly explain that they are not all the same.

@CrabDude
Copy link

@seishun So, IO-bound async (fs.readFile) and CPU-bound async (zlib.deflate). Your 2 links are good references.

When it comes to naming, current APIs prioritize non-blocking (as the default) over faster blocking equivalents in so much that the faster blocking calls require Sync.[1] See @trevnorris' comments on the OP's referenced #1665 for a slight elaboration on this.

My recommendations for any PRs:

  1. Add the following to all Sync calls:

    fs.accessSync(path[, mode])

    Warning: This is a blocking call. See the blocking vs non-blocking guide. (link)

  2. Per @benjamingr suggestion, create a separate guide that concisely covers blocking vs non-blocking and async vs sync while addressing both CPU and IO blocking.

[1] The prioritization order IMO is about identitity. What is node.js' primary value proposition? Is it a _fast_ JavaScript IO-binding runtime? Or a _high-concurrency async non-blocking_ JavaScript IO-binding runtime? Historically, and for the forseeable future, it's definitely the latter, and thus Sync APIs should have the above Warning: ... in their documentation. Non-blocking single-threaded asynchronicity is also The JavaScript Way™ because it lacks all multi-threading support


As an aside, I predict all async counterparts to Sync APIs will eventually be renamed with Async appended in conjunction with the transition toward async/await & Promise and to better distinguish between functions like url.parse and zlib.deflate. Ironically (and intentionally on my part), in this example, the difference is somewhat arbitrary because both are CPU-bound.

@evanlucas
Copy link
Contributor

/cc @nodejs/documentation This is something I feel would be very beneficial for newcomers

@benjamingr
Copy link
Member

Still +1 on this.

@Fishrock123
Copy link
Contributor

Now that we have special ymal metadata maybe we could make this a special field that appears is a specific way with an expandable tooltip? //cc @nodejs/documentation

@Fishrock123 Fishrock123 added the good first issue Issues that are suitable for first-time contributors. label Aug 3, 2016
@Trott
Copy link
Member

Trott commented Jul 9, 2017

@ChALkeR This should remain open?

@wuweiweiwu
Copy link
Contributor

Can I pick this up? I think it'll be a good first issue :)

@benjamingr
Copy link
Member

Actually, this is mostly just confusing new users, I'm going to go ahead and close this (anyone feel free to reopen) given the outcome of #17503

@wuweiweiwu if you're looking for new issues or things to tackle - I think our ESM modules docs could use some love and testing :)

@benjamingr benjamingr removed the good first issue Issues that are suitable for first-time contributors. label Feb 27, 2018
@wuweiweiwu
Copy link
Contributor

@benjamingr Sounds good. Where could I find more information on ESM modules / tests needed to be done? Thank you!

@benjamingr
Copy link
Member

@wuweiweiwu in general, https://github.com/nodejs/node/blob/master/doc/api/esm.md is lacking compared to https://github.com/nodejs/node/blob/master/doc/api/modules.md - I think we should gradually add relevant information and usage examples to the ESM docs

@wuweiweiwu
Copy link
Contributor

@benjamingr Ok! Ill open a WIP PR soon after I read more and get more familiar

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

No branches or pull requests

9 participants