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.

Sign up for GitHub

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

Jump to bottom

src: disable stdio buffering #9203

Closed
wants to merge 1 commit into from
Closed

src: disable stdio buffering #9203

wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link
Member

Cherry-pick of #7610 to v4.x-staging.

R=@thealphanerd

@bnoordhuis
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>
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. v4.x labels Oct 20, 2016
@bnoordhuis bnoordhuis mentioned this pull request Oct 20, 2016
Merged
jasnell
jasnell approved these changes Oct 20, 2016
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM if CI and CITGM are green

thefourtheye
thefourtheye approved these changes Oct 21, 2016
View reviewed changes
Copy link
Contributor

@thefourtheye thefourtheye left a comment

Choose a reason for hiding this comment

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

Same as what @jasnell told

@MylesBorins
Copy link
Contributor

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

@MylesBorins
Copy link
Contributor

one more BSD run https://ci.nodejs.org/job/node-test-commit-freebsd/4976/

@MylesBorins
Copy link
Contributor

BSD 10 is good, BSD 11 is known to be bad

e3e56f5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thefourtheye thefourtheye thefourtheye approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bnoordhuis @MylesBorins @jasnell @thefourtheye @nodejs-github-bot