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

Force line-buffering for stderr #3701

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Nov 6, 2015

Fixes: #2476
Refs: #3615

On CI, it appears that stderr on SmartOS is not line buffered and that was resulting in some interleaving in test-debug-signal-cluster, causing the test to be flaky. This change forces stderr to be line-buffered, fixing the issue.

@Trott Trott added c++ Issues and PRs that require attention from people who are familiar with C++. test Issues and PRs related to the tests. labels Nov 6, 2015
@Trott
Copy link
Member Author

Trott commented Nov 6, 2015

(Is there a better tag/prefix/whatever for this than src:? node:?)

@@ -3129,6 +3129,7 @@ void LoadEnvironment(Environment* env) {
// thrown during process startup.
try_catch.SetVerbose(true);

setvbuf(stderr , NULL , _IOLBF , 1024);
Copy link
Member

Choose a reason for hiding this comment

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

This is not the right place for a call to setvbuf, it's too late after startup. It should probably be the first thing in main() or node::Init() and even that is possibly not soon enough because of printf calls from static constructors.

Copy link
Member Author

Choose a reason for hiding this comment

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

On a separate testing branch, I moved it to main().

I had it run the test on SmartOS 500 times and there were no failures. https://ci.nodejs.org/job/node-test-commit-smartos/246/

Then I commented out the line.
bf88e85

And ran the test again and got multiple failures.
https://ci.nodejs.org/job/node-test-commit-smartos/247/

I'll move the setvbuf() to main() or anywhere else that seems more appropriate.

@Trott
Copy link
Member Author

Trott commented Nov 6, 2015

@@ -41,6 +41,7 @@ int wmain(int argc, wchar_t *wargv[]) {
#else
// UNIX
int main(int argc, char *argv[]) {
setvbuf(stderr , NULL , _IOLBF , 1024);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there shouldn't be a space immediately after each of the first three arguments?

Copy link
Member Author

Choose a reason for hiding this comment

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

Clearly, I need to up my cut-and-paste game. Extraneous spaces removed. Thanks!

@mscdex mscdex removed the test Issues and PRs related to the tests. label Nov 7, 2015
@jbergstroem
Copy link
Member

@Trott should probably be node: $foo.

SmartOS does not line buffer stderr by default, or at least that is the
behavior on the Node project Jenkins server. Force line buffering. This
resolves the flakiness observed on SmartOS for
test-debug-signal-cluster.

Fixes: nodejs#2476
Refs: nodejs#3615
@Trott
Copy link
Member Author

Trott commented Nov 9, 2015

@jbergstroem Cool. Changed the commit message, squashed the commits into a single commit and force pushed.

Anyone feel comfortable giving this one a thumbs up (assuming CI comes back green)?

@jasnell
Copy link
Member

jasnell commented Nov 9, 2015

hmm... would this need to be semver-minor or higher?

@Trott
Copy link
Member Author

Trott commented Nov 9, 2015

@jasnell I don't think so. I think it only affects messages sent by the Node debugger which bypasses console.error() and uses the raw stderr instead.

Anyway, if it increased your comfort level to have it at semver-minor, I have no objection to that. But if you're asking my opinion, I think it's patch level. I just don't hold that opinion particularly strongly.

@bnoordhuis
Copy link
Member

LGTM. This is semver-patch IMO, it only affects fprintf statements in src/, not process.stderr.write statements in JS land.

@jasnell
Copy link
Member

jasnell commented Nov 9, 2015

👍 LGTM

@Trott
Copy link
Member Author

Trott commented Nov 9, 2015

Kewl. I'm waiting on the return of the CI server to do one last rebased-against-master CI run before landing...

@jbergstroem
Copy link
Member

LGTM

@jasnell
Copy link
Member

jasnell commented Nov 10, 2015

@Trott
Copy link
Member Author

Trott commented Nov 10, 2015

OK to land despite OSX + Arm buildbot problems on CI?

@jbergstroem
Copy link
Member

@Trott if you've tested locally on a mac I'd say go for it. I'm +1 to patchlevel.

Trott added a commit to Trott/io.js that referenced this pull request Nov 11, 2015
SmartOS does not line buffer stderr by default, or at least that is the
behavior on the Node project Jenkins server. Force line buffering. This
resolves the flakiness observed on SmartOS for
test-debug-signal-cluster.

PR-URL: nodejs#3701
Fixes: nodejs#2476
Refs: nodejs#3615
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@Trott
Copy link
Member Author

Trott commented Nov 11, 2015

Landed in 0966ab9.

@Trott Trott closed this Nov 11, 2015
Trott added a commit that referenced this pull request Nov 11, 2015
SmartOS does not line buffer stderr by default, or at least that is the
behavior on the Node project Jenkins server. Force line buffering. This
resolves the flakiness observed on SmartOS for
test-debug-signal-cluster.

PR-URL: #3701
Fixes: #2476
Refs: #3615
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@Fishrock123 Fishrock123 mentioned this pull request Nov 11, 2015
@MylesBorins
Copy link
Contributor

should this land in LTS?

/cc @jasnell @bnoordhuis @Trott

@rvagg
Copy link
Member

rvagg commented Dec 1, 2015

hey, this is a good one to discuss at an LTS WG meeting—the change itself is a good one but who knows what weird use is out there that may be depending on existing behaviour, perhaps it's not worth the trouble to backport?

@MylesBorins
Copy link
Contributor

looks like we missed talking about this at todays meeting. @jasnell should we add a label for LTS discussions?

@jasnell
Copy link
Member

jasnell commented Dec 15, 2015

Yeah, if you'd like go ahead and create an lts-agenda label for this repo.
On Dec 14, 2015 4:22 PM, "Myles Borins" notifications@github.com wrote:

looks like we missed talking about this at todays meeting. @jasnell
https://github.com/jasnell should we add a label for LTS discussions?


Reply to this email directly or view it on GitHub
#3701 (comment).

@jasnell
Copy link
Member

jasnell commented Dec 17, 2015

This is flagged as lts-watch but needs to be discussed before landing...

@MylesBorins
Copy link
Contributor

@nodejs/lts can you please weigh in on this one?

@rvagg
Copy link
Member

rvagg commented Feb 18, 2016

From these two comments

@jasnell I don't think so. I think it only affects messages sent by the Node debugger which bypasses console.error() and uses the raw stderr instead.

LGTM. This is semver-patch IMO, it only affects fprintf statements in src/, not process.stderr.write statements in JS land.

If you grep fprintf src/* the impact is pretty low. So +1 from me for LTS.

@jasnell
Copy link
Member

jasnell commented Feb 19, 2016

LGTM to v4.4

MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
SmartOS does not line buffer stderr by default, or at least that is the
behavior on the Node project Jenkins server. Force line buffering. This
resolves the flakiness observed on SmartOS for
test-debug-signal-cluster.

PR-URL: #3701
Fixes: #2476
Refs: #3615
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
SmartOS does not line buffer stderr by default, or at least that is the
behavior on the Node project Jenkins server. Force line buffering. This
resolves the flakiness observed on SmartOS for
test-debug-signal-cluster.

PR-URL: #3701
Fixes: #2476
Refs: #3615
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
SmartOS does not line buffer stderr by default, or at least that is the
behavior on the Node project Jenkins server. Force line buffering. This
resolves the flakiness observed on SmartOS for
test-debug-signal-cluster.

PR-URL: #3701
Fixes: #2476
Refs: #3615
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@Trott Trott deleted the buffer-stderr branch January 13, 2022 22:29
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++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate flaky test test-debug-signal-cluster
7 participants