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

Add two child-process stdio flush tests #7257

Closed
wants to merge 2 commits into from
Closed

Add two child-process stdio flush tests #7257

wants to merge 2 commits into from

Conversation

petrosagg
Copy link

Checklist
  • make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

child-process

Description of change

Fixed a previous testcase that wasn't actually triggering the condition it tried to test. Added a new test that needs #7160 to be merged for it to pass.

@petrosagg
child-process: add stdio flush testcase with pipes
3685d04 
While commit 12274a5 fixed data loss when a readable event handler is
attached, the problem still exists when the stdio stream has a piped
consumer that doesn't read fast enough.

Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
@petrosagg
test: fix testcase to actually detect regression
56d66cf 
In order for the testcase to fail, the calls to read() must be done
after 'exit' is emitted and after flushStdio has been called.

With this change, the testcase will catch any regressions on this
issue.

Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 10, 2016
@petrosagg petrosagg mentioned this pull request Jun 10, 2016
Closed
4 tasks
@addaleax addaleax added child_process Issues and PRs related to the child_process subsystem. lts-watch-v4.x labels Jun 10, 2016
@addaleax
Copy link
Member

@petrosagg Thanks, this is appreciated a lot! It would be great if we could replace spawning seq with another command or e.g. add a seq.js to the test/fixtures directory that implements seq in a cross-platform way. Otherwise, it would probably be best to skip that part of the test on Windows for now.

The subject line of the first commit should also probably start with test: (even though it affects only tests for a specific subsystem).

@addaleax
Copy link
Member

@mscdex The second commit here (the setImmediate change) changes the test case from #5036 so that it fails when executed with node binaries from before that change, which it currently does not afaict, would be great if you could take a look at that.

@mscdex
Copy link
Contributor

mscdex commented Jun 11, 2016

@addaleax I'm not sure what you mean. The existing test (without the setImmediate() addition) definitely does fail on node before the merge of #5036 (for example with node v5.5.0). Adding the setImmediate() does not seem to change things, but if that is the case, why does it need to be added?

@addaleax
Copy link
Member

@mscdex Okay, sorry, I probably got something mixed up – thing is, reverting the lib/ change from #5036 on top of the current master leaves the test working (at least for me locally on Linux). I’ll see if I can figure out what changed in the meantime.

@addaleax
Copy link
Member

@mscdex Okay, so, apparently the order of some child-process-related I/O events changed in libuv/libuv#611, making the test work even without your fix from #5036… would that make sense to you?

@mscdex
Copy link
Contributor

mscdex commented Jun 11, 2016

@addaleax if that's the case, then I'm fine with that change

cjihrig
cjihrig reviewed Jun 13, 2016
View reviewed changes
test/parallel/test-child-process-flush-stdio.js
function spawnWithPipe() {
const buffer = [];
const through = new stream.PassThrough();
const p = cp.spawn('seq', [ '36000' ]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove the spaces inside the square braces.

@addaleax addaleax mentioned this pull request Jun 13, 2016
Closed
@addaleax
Copy link
Member

Initial CI (rebased against master to include #7160): https://ci.nodejs.org/job/node-test-commit/3741/

@addaleax
Copy link
Member

CI is red with timeouts in parallel/test-child-process-flush-stdio on a couple of platforms – not what I would have expected.

@addaleax
Copy link
Member

ping @petrosagg

The test timeouts seem to stem from seq filling up the pipe buffer while the PassThrough is not consuming it, making seq stall and never actually exit (You can test it on Linux by increasing the counter to e.g. 360000).

I’m not sure how that’s best addressed. Killing the child process after a short timeout seems a bit crude, but it’s the most reliable thing that comes to my mind right now.

@addaleax addaleax self-assigned this Jun 23, 2016
@addaleax
Copy link
Member

addaleax commented Jul 4, 2016

@petrosagg Still interested in pursuing this? I’d definitely like to get these tests in in some way, and if you’re not feeling like it, I’d like to offer to take over.

@addaleax addaleax mentioned this pull request Jul 17, 2016
Closed
@rvagg rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02
@jasnell
Copy link
Member

jasnell commented Mar 1, 2017

ping @addaleax ... still interested in pursuing this?

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Mar 1, 2017
@addaleax
Copy link
Member

addaleax commented Mar 1, 2017

yeah let’s close this out, the underlying issues in the streams implementation have been fixed & have tests

@addaleax addaleax closed this Mar 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@addaleax addaleax

Labels
child_process Issues and PRs related to the child_process subsystem. stalled Issues and PRs that are stalled. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@petrosagg @addaleax @mscdex @jasnell @cjihrig @nodejs-github-bot