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

src: disable stdio buffering #7610

Merged
merged 1 commit into from
Jul 11, 2016

Conversation

bnoordhuis
Copy link
Member

Disable stdio buffering, it interacts poorly with printf() calls from
elsewhere in the program (e.g., any logging from V8.) Unbreaks among
other things the --trace_debug_json switch.

Undoes commit 0966ab9 ("src: force line buffering for stderr"), which
in retrospect is not a proper fix. Turning on line buffering fixed a
flaky test on SmartOS but the test wasn't failing on other platforms,
where stderr wasn't line-buffered either.

Disabling buffering should be safe even when mixed with non-blocking
stdio I/O because libuv goes to great lengths to reopen the tty file
descriptors and falls back to blocking I/O when that fails.

R=@Trott?

@bnoordhuis bnoordhuis added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jul 8, 2016
@bnoordhuis
Copy link
Member Author

@MylesBorins
Copy link
Contributor

Should this be back ported to v4?

@Trott
Copy link
Member

Trott commented Jul 8, 2016

Looks like it makes the test flaky again, so at a minimum, I guess this should mark the test flaky again in parallel.status. :-|

@Trott
Copy link
Member

Trott commented Jul 8, 2016

Any chance the parallel/test-vm-sigint failure on FreeBSD in the CI run is related to this change? Doesn't seem like it to me, but ¯_(ツ)_/¯. I haven't seen that one fail before, but it's also a relatively new test. /cc @addaleax https://ci.nodejs.org/job/node-test-commit-freebsd/3201/nodes=freebsd10-64/tapTestReport/test.tap-1065/

@addaleax
Copy link
Member

addaleax commented Jul 8, 2016

@Trott I’d say its unrelated, thanks for pinging me on this, I’ll look into it. :)

@bnoordhuis bnoordhuis force-pushed the disable-stdio-buffering branch from 93aea92 to c2d15cb Compare July 9, 2016 11:45
@bnoordhuis
Copy link
Member Author

Test marked FLAKY. Can I get a LGTM?

@addaleax
Copy link
Member

addaleax commented Jul 9, 2016

LGTM

1 similar comment
@Trott
Copy link
Member

Trott commented Jul 10, 2016

LGTM

@Trott
Copy link
Member

Trott commented Jul 10, 2016

Running CI one more time mostly for confirmation of FreeBSD issue non-relevance: https://ci.nodejs.org/job/node-test-pull-request/3233/

@bnoordhuis
Copy link
Member Author

Java exceptions on the pi1-raspbian-wheezy buildbots. Haven't seen this one before...

java.lang.IllegalStateException: Invalid object ID 214 iota=215

Since they were green last time, I'll land this later today unless someone disagrees.

@Trott
Copy link
Member

Trott commented Jul 10, 2016

:shipit:

Disable stdio buffering, it interacts poorly with printf() calls from
elsewhere in the program (e.g., any logging from V8.)  Unbreaks among
other things the `--trace_debug_json` switch.

Undoes commit 0966ab9 ("src: force line buffering for stderr"), which
in retrospect is not a proper fix.  Turning on line buffering fixed a
flaky test on SmartOS but the test wasn't failing on other platforms,
where stderr wasn't line-buffered either.  Mark the test flaky again,
it failed once in a run of 333 tries on the smartos-64 buildbot.

Disabling buffering should be safe even when mixed with non-blocking
stdio I/O because libuv goes to great lengths to reopen the tty file
descriptors and falls back to blocking I/O when that fails.

PR-URL: nodejs#7610
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@bnoordhuis bnoordhuis force-pushed the disable-stdio-buffering branch from c2d15cb to cfe76f2 Compare July 11, 2016 21:17
@bnoordhuis bnoordhuis closed this Jul 11, 2016
@bnoordhuis bnoordhuis deleted the disable-stdio-buffering branch July 11, 2016 21:17
@bnoordhuis bnoordhuis merged commit cfe76f2 into nodejs:master Jul 11, 2016
@bnoordhuis
Copy link
Member Author

@thealphanerd Eventually, but let's give it a little time. I'd like to see it go into one or two v6 releases first, just to check if it doesn't break anything.

@MylesBorins
Copy link
Contributor

sgtm @bnoordhuis

evanlucas pushed a commit that referenced this pull request Jul 15, 2016
Disable stdio buffering, it interacts poorly with printf() calls from
elsewhere in the program (e.g., any logging from V8.)  Unbreaks among
other things the `--trace_debug_json` switch.

Undoes commit 0966ab9 ("src: force line buffering for stderr"), which
in retrospect is not a proper fix.  Turning on line buffering fixed a
flaky test on SmartOS but the test wasn't failing on other platforms,
where stderr wasn't line-buffered either.  Mark the test flaky again,
it failed once in a run of 333 tries on the smartos-64 buildbot.

Disabling buffering should be safe even when mixed with non-blocking
stdio I/O because libuv goes to great lengths to reopen the tty file
descriptors and falls back to blocking I/O when that fails.

PR-URL: #7610
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins
Copy link
Contributor

ping @bnoordhuis

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Oct 20, 2016
Disable stdio buffering, it interacts poorly with printf() calls from
elsewhere in the program (e.g., any logging from V8.)  Unbreaks among
other things the `--trace_debug_json` switch.

Undoes commit 0966ab9 ("src: force line buffering for stderr"), which
in retrospect is not a proper fix.  Turning on line buffering fixed a
flaky test on SmartOS but the test wasn't failing on other platforms,
where stderr wasn't line-buffered either.  Mark the test flaky again,
it failed once in a run of 333 tries on the smartos-64 buildbot.

Disabling buffering should be safe even when mixed with non-blocking
stdio I/O because libuv goes to great lengths to reopen the tty file
descriptors and falls back to blocking I/O when that fails.

PR-URL: nodejs#7610
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@bnoordhuis
Copy link
Member Author

#9203 - cherry-pick to v4.x.

MylesBorins pushed a commit that referenced this pull request Oct 24, 2016
Disable stdio buffering, it interacts poorly with printf() calls from
elsewhere in the program (e.g., any logging from V8.)  Unbreaks among
other things the `--trace_debug_json` switch.

Undoes commit 0966ab9 ("src: force line buffering for stderr"), which
in retrospect is not a proper fix.  Turning on line buffering fixed a
flaky test on SmartOS but the test wasn't failing on other platforms,
where stderr wasn't line-buffered either.  Mark the test flaky again,
it failed once in a run of 333 tries on the smartos-64 buildbot.

Disabling buffering should be safe even when mixed with non-blocking
stdio I/O because libuv goes to great lengths to reopen the tty file
descriptors and falls back to blocking I/O when that fails.

PR-URL: #7610
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
Disable stdio buffering, it interacts poorly with printf() calls from
elsewhere in the program (e.g., any logging from V8.)  Unbreaks among
other things the `--trace_debug_json` switch.

Undoes commit 0966ab9 ("src: force line buffering for stderr"), which
in retrospect is not a proper fix.  Turning on line buffering fixed a
flaky test on SmartOS but the test wasn't failing on other platforms,
where stderr wasn't line-buffered either.  Mark the test flaky again,
it failed once in a run of 333 tries on the smartos-64 buildbot.

Disabling buffering should be safe even when mixed with non-blocking
stdio I/O because libuv goes to great lengths to reopen the tty file
descriptors and falls back to blocking I/O when that fails.

PR-URL: #7610
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants