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

net: make stdout & stderr block on all platforms #1771

Closed
wants to merge 2 commits into from

Conversation

Fishrock123
Copy link
Contributor

Fixes #784

@vkurchatkin suggested a test here, but I'm not sure how to adapt to be only in javascript. Maybe with two child processes? I'm not sure how to pipe them without the unix |.

R=@bnoordhuis

@Fishrock123 Fishrock123 added the net Issues and PRs related to the net subsystem. label May 22, 2015
(this._handle instanceof Pipe) &&
process.platform === 'win32') {
// Make stdout and stderr blocking on Windows
(this._handle instanceof Pipe) {
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look like valid JS to me (mismatched parentheses.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doh, forgot to run the tests.

@bnoordhuis
Copy link
Member

Maybe with two child processes?

Yes, that should work.

@Fishrock123
Copy link
Contributor Author

Oh, I could just pipe the stdout from one to the sdin of the other via .pipe, I think. Also doh.

@Fishrock123
Copy link
Contributor Author

@bnoordhuis updated ptal

const add = 'test\n';

var str = '';
for (var i = 0; i < 100000; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: use 1e5 here. Personally, I find scientific notation easier to read for large numbers.

@@ -9,6 +9,8 @@ ecmaFeatures:
binaryLiterals: true
generators: true
forOf: true
objectLiteralShorthandMethods: true
objectLiteralShorthandProperties: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

used here: https://github.com/nodejs/io.js/pull/1771/files#diff-7718507eb7a71106a68401cae76c5268R14

I don't see a good reason not to enable these other than backporting, maybe.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, we had to enable one of these in #1760 already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I only needed one here, but might as well enable both.

@bnoordhuis
Copy link
Member

LGTM if the CI is happy.

@Fishrock123
Copy link
Contributor Author

@Fishrock123
Copy link
Contributor Author

Smartos seems unrelated, will land tomorrow.

@bnoordhuis
Copy link
Member

One final suggestion: maybe you can go into a little more detail in the commit log about the ramifications of this change.

@Fishrock123
Copy link
Contributor Author

@bnoordhuis updated, ptal at code comment(s) + commit message. I think I'm correct in what is happening.

.. Although, I'm a little less sure this is actually a bug rather than expected behaviour when the process exits "early"? Again, it doesn't fail without the text producer exiting it early.

@Fishrock123
Copy link
Contributor Author

ping @bnoordhuis

@Fishrock123
Copy link
Contributor Author

See also #1541

(this._handle instanceof Pipe) &&
process.platform === 'win32') {
// Make stdout and stderr blocking on Windows
this._handle instanceof Pipe) {
Copy link
Member

Choose a reason for hiding this comment

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

Fits on a single line now, I think?

@bnoordhuis
Copy link
Member

LGTM with comments. Can you rebase and run the CI?

@Fishrock123 Fishrock123 mentioned this pull request May 20, 2016
4 tasks
Fishrock123 added a commit to Fishrock123/node that referenced this pull request May 26, 2016
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Jun 1, 2016
OS X has a tiny 1kb hard-coded buffer size for stdout / stderr to
TTYs (terminals). Output larger than that causes chunking, which ends
up having some (very small but existent) delay past the first chunk.
That causes two problems:

1. When output is written to stdout and stderr at similar times, the
two can become mixed together (interleaved). This is especially
problematic when using control characters, such as \r. With
interleaving, chunked output will often have lines or characters erased
unintentionally, or in the wrong spots, leading to broken output.
CLI apps often extensively use such characters for things such as
progress bars.

2. Output can be lost if the process is exited before chunked writes
are finished flushing. This usually happens in applications that use
`process.exit()`, which isn't infrequent.

See nodejs#6980 for more info.

This became an issue as result of the Libuv 1.9.0 upgrade. A fix to
an unrelated issue broke a hack previously required for the OS X
implementation. This resulted in an unexpected behavior change in node.
The 1.9.0 upgrade was done in c3cec1e,
which was included in v6.0.0.
Full details of the Libuv issue that induced this are at
nodejs#6456 (comment)

Refs: nodejs#1771
Refs: nodejs#6456
Refs: nodejs#6773
Refs: nodejs#6816
PR-URL: nodejs#6895
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Fishrock123 added a commit that referenced this pull request Jun 1, 2016
OS X has a tiny 1kb hard-coded buffer size for stdout / stderr to
TTYs (terminals). Output larger than that causes chunking, which ends
up having some (very small but existent) delay past the first chunk.
That causes two problems:

1. When output is written to stdout and stderr at similar times, the
two can become mixed together (interleaved). This is especially
problematic when using control characters, such as \r. With
interleaving, chunked output will often have lines or characters erased
unintentionally, or in the wrong spots, leading to broken output.
CLI apps often extensively use such characters for things such as
progress bars.

2. Output can be lost if the process is exited before chunked writes
are finished flushing. This usually happens in applications that use
`process.exit()`, which isn't infrequent.

See #6980 for more info.

This became an issue as result of the Libuv 1.9.0 upgrade. A fix to
an unrelated issue broke a hack previously required for the OS X
implementation. This resulted in an unexpected behavior change in node.
The 1.9.0 upgrade was done in c3cec1e,
which was included in v6.0.0.
Full details of the Libuv issue that induced this are at
#6456 (comment)

Refs: #1771
Refs: #6456
Refs: #6773
Refs: #6816
PR-URL: #6895
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
rvagg pushed a commit that referenced this pull request Jun 2, 2016
OS X has a tiny 1kb hard-coded buffer size for stdout / stderr to
TTYs (terminals). Output larger than that causes chunking, which ends
up having some (very small but existent) delay past the first chunk.
That causes two problems:

1. When output is written to stdout and stderr at similar times, the
two can become mixed together (interleaved). This is especially
problematic when using control characters, such as \r. With
interleaving, chunked output will often have lines or characters erased
unintentionally, or in the wrong spots, leading to broken output.
CLI apps often extensively use such characters for things such as
progress bars.

2. Output can be lost if the process is exited before chunked writes
are finished flushing. This usually happens in applications that use
`process.exit()`, which isn't infrequent.

See #6980 for more info.

This became an issue as result of the Libuv 1.9.0 upgrade. A fix to
an unrelated issue broke a hack previously required for the OS X
implementation. This resulted in an unexpected behavior change in node.
The 1.9.0 upgrade was done in c3cec1e,
which was included in v6.0.0.
Full details of the Libuv issue that induced this are at
#6456 (comment)

Refs: #1771
Refs: #6456
Refs: #6773
Refs: #6816
PR-URL: #6895
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
saghul pushed a commit to saghul/node that referenced this pull request Jul 11, 2016
OS X has a tiny 1kb hard-coded buffer size for stdout / stderr to
TTYs (terminals). Output larger than that causes chunking, which ends
up having some (very small but existent) delay past the first chunk.
That causes two problems:

1. When output is written to stdout and stderr at similar times, the
two can become mixed together (interleaved). This is especially
problematic when using control characters, such as \r. With
interleaving, chunked output will often have lines or characters erased
unintentionally, or in the wrong spots, leading to broken output.
CLI apps often extensively use such characters for things such as
progress bars.

2. Output can be lost if the process is exited before chunked writes
are finished flushing. This usually happens in applications that use
`process.exit()`, which isn't infrequent.

See nodejs#6980 for more info.

This became an issue as result of the Libuv 1.9.0 upgrade. A fix to
an unrelated issue broke a hack previously required for the OS X
implementation. This resulted in an unexpected behavior change in node.
The 1.9.0 upgrade was done in c3cec1e,
which was included in v6.0.0.
Full details of the Libuv issue that induced this are at
nodejs#6456 (comment)

Refs: nodejs#1771
Refs: nodejs#6456
Refs: nodejs#6773
Refs: nodejs#6816
PR-URL: nodejs#6895
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
OS X has a tiny 1kb hard-coded buffer size for stdout / stderr to
TTYs (terminals). Output larger than that causes chunking, which ends
up having some (very small but existent) delay past the first chunk.
That causes two problems:

1. When output is written to stdout and stderr at similar times, the
two can become mixed together (interleaved). This is especially
problematic when using control characters, such as \r. With
interleaving, chunked output will often have lines or characters erased
unintentionally, or in the wrong spots, leading to broken output.
CLI apps often extensively use such characters for things such as
progress bars.

2. Output can be lost if the process is exited before chunked writes
are finished flushing. This usually happens in applications that use
`process.exit()`, which isn't infrequent.

See #6980 for more info.

This became an issue as result of the Libuv 1.9.0 upgrade. A fix to
an unrelated issue broke a hack previously required for the OS X
implementation. This resulted in an unexpected behavior change in node.
The 1.9.0 upgrade was done in c3cec1e,
which was included in v6.0.0.
Full details of the Libuv issue that induced this are at
#6456 (comment)

Refs: #1771
Refs: #6456
Refs: #6773
Refs: #6816
PR-URL: #6895
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
OS X has a tiny 1kb hard-coded buffer size for stdout / stderr to
TTYs (terminals). Output larger than that causes chunking, which ends
up having some (very small but existent) delay past the first chunk.
That causes two problems:

1. When output is written to stdout and stderr at similar times, the
two can become mixed together (interleaved). This is especially
problematic when using control characters, such as \r. With
interleaving, chunked output will often have lines or characters erased
unintentionally, or in the wrong spots, leading to broken output.
CLI apps often extensively use such characters for things such as
progress bars.

2. Output can be lost if the process is exited before chunked writes
are finished flushing. This usually happens in applications that use
`process.exit()`, which isn't infrequent.

See #6980 for more info.

This became an issue as result of the Libuv 1.9.0 upgrade. A fix to
an unrelated issue broke a hack previously required for the OS X
implementation. This resulted in an unexpected behavior change in node.
The 1.9.0 upgrade was done in c3cec1e,
which was included in v6.0.0.
Full details of the Libuv issue that induced this are at
#6456 (comment)

Refs: #1771
Refs: #6456
Refs: #6773
Refs: #6816
PR-URL: #6895
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
OS X has a tiny 1kb hard-coded buffer size for stdout / stderr to
TTYs (terminals). Output larger than that causes chunking, which ends
up having some (very small but existent) delay past the first chunk.
That causes two problems:

1. When output is written to stdout and stderr at similar times, the
two can become mixed together (interleaved). This is especially
problematic when using control characters, such as \r. With
interleaving, chunked output will often have lines or characters erased
unintentionally, or in the wrong spots, leading to broken output.
CLI apps often extensively use such characters for things such as
progress bars.

2. Output can be lost if the process is exited before chunked writes
are finished flushing. This usually happens in applications that use
`process.exit()`, which isn't infrequent.

See #6980 for more info.

This became an issue as result of the Libuv 1.9.0 upgrade. A fix to
an unrelated issue broke a hack previously required for the OS X
implementation. This resulted in an unexpected behavior change in node.
The 1.9.0 upgrade was done in c3cec1e,
which was included in v6.0.0.
Full details of the Libuv issue that induced this are at
#6456 (comment)

Refs: #1771
Refs: #6456
Refs: #6773
Refs: #6816
PR-URL: #6895
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
OS X has a tiny 1kb hard-coded buffer size for stdout / stderr to
TTYs (terminals). Output larger than that causes chunking, which ends
up having some (very small but existent) delay past the first chunk.
That causes two problems:

1. When output is written to stdout and stderr at similar times, the
two can become mixed together (interleaved). This is especially
problematic when using control characters, such as \r. With
interleaving, chunked output will often have lines or characters erased
unintentionally, or in the wrong spots, leading to broken output.
CLI apps often extensively use such characters for things such as
progress bars.

2. Output can be lost if the process is exited before chunked writes
are finished flushing. This usually happens in applications that use
`process.exit()`, which isn't infrequent.

See #6980 for more info.

This became an issue as result of the Libuv 1.9.0 upgrade. A fix to
an unrelated issue broke a hack previously required for the OS X
implementation. This resulted in an unexpected behavior change in node.
The 1.9.0 upgrade was done in c3cec1e,
which was included in v6.0.0.
Full details of the Libuv issue that induced this are at
#6456 (comment)

Refs: #1771
Refs: #6456
Refs: #6773
Refs: #6816
PR-URL: #6895
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
OS X has a tiny 1kb hard-coded buffer size for stdout / stderr to
TTYs (terminals). Output larger than that causes chunking, which ends
up having some (very small but existent) delay past the first chunk.
That causes two problems:

1. When output is written to stdout and stderr at similar times, the
two can become mixed together (interleaved). This is especially
problematic when using control characters, such as \r. With
interleaving, chunked output will often have lines or characters erased
unintentionally, or in the wrong spots, leading to broken output.
CLI apps often extensively use such characters for things such as
progress bars.

2. Output can be lost if the process is exited before chunked writes
are finished flushing. This usually happens in applications that use
`process.exit()`, which isn't infrequent.

See #6980 for more info.

This became an issue as result of the Libuv 1.9.0 upgrade. A fix to
an unrelated issue broke a hack previously required for the OS X
implementation. This resulted in an unexpected behavior change in node.
The 1.9.0 upgrade was done in c3cec1e,
which was included in v6.0.0.
Full details of the Libuv issue that induced this are at
#6456 (comment)

Refs: #1771
Refs: #6456
Refs: #6773
Refs: #6816
PR-URL: #6895
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Jul 18, 2016
addaleax pushed a commit that referenced this pull request Aug 10, 2016
Refs: #1771
Refs: #6456
Refs: #6773
Refs: #7743
PR-URL: #6816
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Saúl Ibarra Corretgé <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
cjihrig pushed a commit that referenced this pull request Aug 11, 2016
Refs: #1771
Refs: #6456
Refs: #6773
Refs: #7743
PR-URL: #6816
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Saúl Ibarra Corretgé <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chunked stdout/stderr drops writes if terminated early.
7 participants