doc: warn against streaming from character devices #21212

gireeshpunathil · 2018-06-08T14:28:24Z

charcter device streaming works just like any other streams, but hangs
on the close callsite due to the worker thread being blocked on the read
and main thread waiting for any async event that may not occur.

Document this behavior and suggest a potential workaround.

Fixes: #15439

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

Trott · 2018-06-09T05:10:04Z

doc/api/fs.md

@@ -1381,6 +1381,12 @@ the specified file descriptor. This means that no `'open'` event will be
 emitted. Note that `fd` should be blocking; non-blocking `fd`s should be passed
 to [`net.Socket`][].

+Also note that the blocking fd, if pointing to a character device (such as
+keyboard or sound card) that are end-less streams and do not produce EOF


Nit: end-less -> endless

Trott · 2018-06-09T05:10:22Z

@nodejs/fs @nodejs/documentation

gireeshpunathil · 2018-06-11T07:23:51Z

doc/api/fs.md

@@ -1381,6 +1381,12 @@ the specified file descriptor. This means that no `'open'` event will be
 emitted. Note that `fd` should be blocking; non-blocking `fd`s should be passed
 to [`net.Socket`][].

+Also note that the blocking fd, if pointing to a character device (such as


@vsemozhetbyt - split it for more clarity, ptal.

"fd" should be in backticks, I think. (Can go either way, but for consistency with the previous paragraph, for example.)

You can probably drop Also note that.

Trott

Just putting a 'request changes' here to make sure this doesn't land without @vsemozhetbyt's comment being addressed. If the comment gets addressed, feel free to dismiss this review.

vsemozhetbyt

With nits)

vsemozhetbyt · 2018-06-11T15:27:44Z

doc/api/fs.md

+keyboard or sound card) can potentially block the main thread on stream close.
+This is because these devices do not produce EOF character as part of their data
+flow cycle, and thereby epitomize endless streams. As a result, they do not
+respond to stream.close() call. A workaround is to close the stream first using


stream.close() -> `stream.close()`?

vsemozhetbyt · 2018-06-11T15:27:58Z

doc/api/fs.md

+This is because these devices do not produce EOF character as part of their data
+flow cycle, and thereby epitomize endless streams. As a result, they do not
+respond to stream.close() call. A workaround is to close the stream first using
+stream.close() and then push a random character into the stream. This unblocks


stream.close() -> `stream.close()`?

gireeshpunathil · 2018-06-12T05:48:28Z

@vsemozhetbyt - thanks, nits addressed.

mafintosh · 2018-06-12T14:12:37Z

doc/api/fs.md

+Also note that the blocking fd, if pointing to a character device (such as
+keyboard or sound card) can potentially block the main thread on stream close.
+This is because these devices do not produce EOF character as part of their data
+flow cycle, and thereby epitomize endless streams. As a result, they do not


I don't think "epitomize" is a word most non-english speakers will know.

please suggest alternative, if you know.

just endless streams straight up? i don't personally know what epitomize mean

changed the wording from epitomize to exemplifies

mafintosh · 2018-06-12T14:13:55Z

doc/api/fs.md

+respond to `stream.close()` call. A workaround is to close the stream first
+using `stream.close()` and then push a random character into the stream.
+This unblocks the reader thread, leads to the completion of the data flow cycle,
+and the actual closing of the stream.


I'm not sure I follow this last sentence.

the problem with character device when abstracted as streams in Node is that they operate on a blocking file descriptor, managed by pooled worker threads, with a passback async handle to the main thread. so once they engage in reading, there is no way to unblock the thread, other than passing a dummy character to 'unblock' the reader thread, close the libuv handles and disengage from further reading.

Is that something you can do with the fs api here? An example might work to reduce confusion :)

added an example

vsemozhetbyt · 2018-06-12T15:42:24Z

doc/api/fs.md

+
+```js
+const fs = require('fs');
+// create a stream from some character  device


Nit: it seems we usually start comments with an uppercase letter and end them with a period.

thanks, done.

reason for this change request was addressed, hence dismissing

gireeshpunathil · 2018-06-22T10:34:15Z

@mafintosh @Trott - all comments were addressed, PTAL. thanks.

jasnell · 2018-08-12T07:31:54Z

ping @mafintosh @Trott

Trott · 2018-08-13T18:51:12Z

doc/api/fs.md

+keyboard or sound card) can potentially block the main thread on stream close.
+This is because these devices do not produce EOF character as part of their data
+flow cycle, and thereby exemplify endless streams. As a result, they do not
+respond to `stream.close()` call. A workaround is to close the stream first


Omit call?

Trott · 2018-08-13T18:51:35Z

doc/api/fs.md

+This is because these devices do not produce EOF character as part of their data
+flow cycle, and thereby exemplify endless streams. As a result, they do not
+respond to `stream.close()` call. A workaround is to close the stream first
+using `stream.close()` and then push a random character into the stream, issue


issue -> and issue or issuing?

Trott · 2018-08-13T18:52:48Z

Left some non-blocking nits. I'm fine with this landing, but am not leaving an approval because the behavior described is not something I'm familiar with. I trust that it is being accurately described, though.

charcter device streaming works just like any other streams, but hangs on the close callsite due to the worker thread being blocked on the read and main thread waiting for any async event that may not occur. Document this behavior and suggest a potential workaround. Fixes: nodejs#15439

gireeshpunathil · 2018-08-14T07:24:24Z

thanks @Trott , I have addressed all your comments.

gireeshpunathil · 2018-08-28T13:27:50Z

landed as 66e6d78

charcter device streaming works just like any other streams, but hangs on the close callsite due to the worker thread being blocked on the read and main thread waiting for any async event that may not occur. Document this behavior and suggest a potential workaround. Fixes: #15439 PR-URL: #21212 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

The text contained a number of inaccuracies: - The main thread is never blocked by a stream close. - There is no such thing as an EOF character on the OS level, the libuv level, or the Node.js stream level. - These streams *do* respond to `.close()`, but pending reads can delay this indefinitely. - Pushing a random character into the stream works only when the source data can be controlled; Using the JS `.push()` method is something different, and does not “unblock” any threads. Refs: nodejs#21212

charcter device streaming works just like any other streams, but hangs on the close callsite due to the worker thread being blocked on the read and main thread waiting for any async event that may not occur. Document this behavior and suggest a potential workaround. Fixes: #15439 PR-URL: #21212 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

The text contained a number of inaccuracies: - The main thread is never blocked by a stream close. - There is no such thing as an EOF character on the OS level, the libuv level, or the Node.js stream level. - These streams *do* respond to `.close()`, but pending reads can delay this indefinitely. - Pushing a random character into the stream works only when the source data can be controlled; Using the JS `.push()` method is something different, and does not “unblock” any threads. Refs: #21212 PR-URL: #22569 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

charcter device streaming works just like any other streams, but hangs on the close callsite due to the worker thread being blocked on the read and main thread waiting for any async event that may not occur. Document this behavior and suggest a potential workaround. Fixes: #15439 PR-URL: #21212 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

The text contained a number of inaccuracies: - The main thread is never blocked by a stream close. - There is no such thing as an EOF character on the OS level, the libuv level, or the Node.js stream level. - These streams *do* respond to `.close()`, but pending reads can delay this indefinitely. - Pushing a random character into the stream works only when the source data can be controlled; Using the JS `.push()` method is something different, and does not “unblock” any threads. Refs: #21212 PR-URL: #22569 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

charcter device streaming works just like any other streams, but hangs on the close callsite due to the worker thread being blocked on the read and main thread waiting for any async event that may not occur. Document this behavior and suggest a potential workaround. Fixes: #15439 PR-URL: #21212 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

The text contained a number of inaccuracies: - The main thread is never blocked by a stream close. - There is no such thing as an EOF character on the OS level, the libuv level, or the Node.js stream level. - These streams *do* respond to `.close()`, but pending reads can delay this indefinitely. - Pushing a random character into the stream works only when the source data can be controlled; Using the JS `.push()` method is something different, and does not “unblock” any threads. Refs: #21212 PR-URL: #22569 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

nodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Jun 8, 2018

jasnell requested a review from mafintosh June 9, 2018 01:00

Trott reviewed Jun 9, 2018

View reviewed changes

vsemozhetbyt reviewed Jun 9, 2018

View reviewed changes

jasnell approved these changes Jun 10, 2018

View reviewed changes

Trott previously requested changes Jun 10, 2018

View reviewed changes

gireeshpunathil force-pushed the char-device-block-behavior branch from 15a20ac to d0aa7ed Compare June 11, 2018 07:22

vsemozhetbyt approved these changes Jun 11, 2018

View reviewed changes

gireeshpunathil force-pushed the char-device-block-behavior branch from 2448d2d to 040f3d5 Compare June 12, 2018 05:47

mafintosh reviewed Jun 12, 2018

View reviewed changes

gireeshpunathil force-pushed the char-device-block-behavior branch from f48adc2 to f1f4dfc Compare June 12, 2018 15:27

vsemozhetbyt reviewed Jun 12, 2018

View reviewed changes

gireeshpunathil force-pushed the char-device-block-behavior branch from 58c22ab to fc01bd8 Compare June 13, 2018 08:26

gireeshpunathil mentioned this pull request Jun 14, 2018

readline with /dev/tty: echos and doesn't close #21319

Open

trivikr approved these changes Aug 12, 2018

View reviewed changes

Trott reviewed Aug 13, 2018

View reviewed changes

gireeshpunathil added 2 commits August 14, 2018 12:47

address review comments

9226e41

gireeshpunathil force-pushed the char-device-block-behavior branch from fc01bd8 to 9226e41 Compare August 14, 2018 07:21

gireeshpunathil closed this Aug 28, 2018

addaleax mentioned this pull request Aug 28, 2018

doc: fix up warning text about character devices #22569

Closed

3 tasks

targos mentioned this pull request Sep 5, 2018

Release proposal: v10.10.0 #22716

Merged

This was referenced Sep 6, 2018

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-android-nodejs#236

Closed

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-nodejs#221

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: warn against streaming from character devices #21212

doc: warn against streaming from character devices #21212

gireeshpunathil commented Jun 8, 2018

Trott Jun 9, 2018

gireeshpunathil Jun 11, 2018

Trott commented Jun 9, 2018

This comment was marked as resolved.

gireeshpunathil Jun 11, 2018

Trott Aug 13, 2018

Trott left a comment

vsemozhetbyt left a comment

vsemozhetbyt Jun 11, 2018

vsemozhetbyt Jun 11, 2018

gireeshpunathil commented Jun 12, 2018

mafintosh Jun 12, 2018

gireeshpunathil Jun 12, 2018

mafintosh Jun 12, 2018

gireeshpunathil Jun 12, 2018

mafintosh Jun 12, 2018

gireeshpunathil Jun 12, 2018

mafintosh Jun 12, 2018

gireeshpunathil Jun 12, 2018

vsemozhetbyt Jun 12, 2018

gireeshpunathil Jun 13, 2018

gireeshpunathil commented Jun 22, 2018

jasnell commented Aug 12, 2018

Trott Aug 13, 2018

Trott Aug 13, 2018

Trott commented Aug 13, 2018 •

edited

Loading

gireeshpunathil commented Aug 14, 2018

gireeshpunathil commented Aug 28, 2018

doc: warn against streaming from character devices #21212

doc: warn against streaming from character devices #21212

Conversation

gireeshpunathil commented Jun 8, 2018

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Jun 9, 2018

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott left a comment

Choose a reason for hiding this comment

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gireeshpunathil commented Jun 12, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gireeshpunathil commented Jun 22, 2018

jasnell commented Aug 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Aug 13, 2018 • edited Loading

gireeshpunathil commented Aug 14, 2018

gireeshpunathil commented Aug 28, 2018

Trott commented Aug 13, 2018 •

edited

Loading