FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: disable stdio buffering by bnoordhuis · Pull Request #7610 · nodejs/node · GitHub

/ node Public

src: disable stdio buffering - #7610

Merged
bnoordhuis merged 1 commit into
nodejs:masterfrom
bnoordhuis:disable-stdio-buffering
Jul 11, 2016
ConversationCommits (1)ChecksFiles changed
Merged

src: disable stdio buffering#7610
bnoordhuis merged 1 commit into
nodejs:masterfrom
bnoordhuis:disable-stdio-buffering

Conversation

bnoordhuis commented Jul 8, 2016

Copy link
Copy Markdown
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 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 commented Jul 8, 2016

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/3216/

smartos-64 stress test: https://ci.nodejs.org/job/node-stress-single-test/794/

MylesBorins commented Jul 8, 2016

Copy link
Copy Markdown
Contributor

Should this be back ported to v4?

Trott commented Jul 8, 2016

Copy link
Copy Markdown
Member

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 commented Jul 8, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

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 commented Jul 8, 2016

Copy link
Copy Markdown
Member

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

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

bnoordhuis commented Jul 9, 2016

Copy link
Copy Markdown
Member Author

Test marked FLAKY. Can I get a LGTM?

addaleax commented Jul 9, 2016

Copy link
Copy Markdown
Member

LGTM

1 similar comment

Trott commented Jul 10, 2016

Copy link
Copy Markdown
Member

LGTM

Trott commented Jul 10, 2016

Copy link
Copy Markdown
Member

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

bnoordhuis commented Jul 10, 2016

Copy link
Copy Markdown
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 commented Jul 10, 2016

Copy link
Copy Markdown
Member

src: disable stdio buffering
cfe76f2 
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 force-pushed the disable-stdio-buffering branch from c2d15cb to cfe76f2 Compare July 11, 2016 21:17
bnoordhuis closed this Jul 11, 2016
bnoordhuis deleted the disable-stdio-buffering branch July 11, 2016 21:17
bnoordhuis merged commit cfe76f2 into nodejs:master Jul 11, 2016

bnoordhuis commented Jul 11, 2016

Copy link
Copy Markdown
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 commented Jul 11, 2016

Copy link
Copy Markdown
Contributor

sgtm @bnoordhuis

evanlucas pushed a commit that referenced this pull request Jul 15, 2016
src: disable stdio buffering
fbc9ef8 
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>
evanlucas mentioned this pull request Jul 18, 2016
Merged

MylesBorins commented Oct 10, 2016

Copy link
Copy Markdown
Contributor

ping @bnoordhuis

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Oct 20, 2016
src: disable stdio buffering
3187d3f 
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 mentioned this pull request Oct 20, 2016
Closed

bnoordhuis commented Oct 20, 2016

Copy link
Copy Markdown
Member Author

#9203 - cherry-pick to v4.x.

MylesBorins pushed a commit that referenced this pull request Oct 24, 2016
src: disable stdio buffering
e3e56f5 
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
src: disable stdio buffering
8b93fdd 
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 mentioned this pull request Oct 26, 2016
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL