Force line-buffering for stderr#3701
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Trott added c++ 
Trott commented Nov 6, 2015
(Is there a better tag/prefix/whatever for this than |
src/node.cc Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 commented Nov 6, 2015
src/node_main.cc Outdated
There was a problem hiding this comment.
I think there shouldn't be a space immediately after each of the first three arguments?
There was a problem hiding this comment.
Clearly, I need to up my cut-and-paste game. Extraneous spaces removed. Thanks!
jbergstroem commented Nov 9, 2015
@Trott should probably be |
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 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 commented Nov 9, 2015
hmm... would this need to be semver-minor or higher? |
Trott commented Nov 9, 2015
@jasnell I don't think so. I think it only affects messages sent by the Node debugger which bypasses Anyway, if it increased your comfort level to have it at |
bnoordhuis commented Nov 9, 2015
LGTM. This is semver-patch IMO, it only affects fprintf statements in src/, not |
jasnell commented Nov 9, 2015
👍 LGTM |
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 commented Nov 10, 2015
LGTM |
jasnell commented Nov 10, 2015
Trott commented Nov 10, 2015
OK to land despite OSX + Arm buildbot problems on CI? |
jbergstroem commented Nov 11, 2015
@Trott if you've tested locally on a mac I'd say go for it. I'm +1 to patchlevel. |
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#3701Fixes: nodejs#2476 Refs: nodejs#3615 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
Trott commented Nov 11, 2015
Landed in 0966ab9. |
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: #3701Fixes: #2476 Refs: #3615 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins commented Dec 1, 2015
should this land in LTS? |
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 commented Dec 15, 2015
looks like we missed talking about this at todays meeting. @jasnell should we add a label for LTS discussions? |
jasnell commented Dec 15, 2015
Yeah, if you'd like go ahead and create an lts-agenda label for this repo.
|
jasnell commented Dec 17, 2015
This is flagged as lts-watch but needs to be discussed before landing... |
MylesBorins commented Feb 17, 2016
@nodejs/lts can you please weigh in on this one? |
rvagg commented Feb 18, 2016
From these two comments
If you |
jasnell commented Feb 19, 2016
LGTM to v4.4 |
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: #3701Fixes: #2476 Refs: #3615 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
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: #3701Fixes: #2476 Refs: #3615 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
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: #3701Fixes: #2476 Refs: #3615 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
7 participants
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.