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

test: share maxBuffer between stdout and stderr #4035

Closed
wants to merge 1 commit into from
Closed

test: share maxBuffer between stdout and stderr #4035

wants to merge 1 commit into from

Conversation

gireeshpunathil
Copy link
Member

The last item in this test case - with maxBuffer set to 1 fails in AIX, with an assertion failure. Root cause is that the chid's stdout file is not populated as expected.

Given that:
a) While spawnSync() API is blocking, the code within the spawned child is still asynchronous.
b) While maxBuffer is the limit of data allowed for child's stdout + stderr together, and if exceeded child process is killed, there is no dictum as to who can consume this up.

It is evident that console.error() got processed first before console.log in the child. While the call was made in order, their descriptors became IO ready in the reverse order.
Consequently, stderr populated its data into the channel buffer, and crossed the maxBuffer (maxBuffer is lazy, overflow check performed post-write), and caused the child termination, short-circuiting stdout's event. Tracing at the readCallback() for the streams (the first interception point behind the uv loop), I clearly see the order reversal.

While this is not happening in Linux or Windows, no documented evidence exists for the order of event rediness in the event loop.

In summary, the test should not assume that the buffer would be consumed first by stdout.

The test case assumes that child's stdout should be holding the data
as prescribed by the child's codeflow. However, as the child code runs
asynchronously, this order can be reversed.

Assertion fails when console.error() was first processed in the child.
While the call was made in order, their fd's became IO ready in
the reverse order. Consequently, stderr populated its data into the
channel buffer, and crossed the maxBuffer and caused the child
termination, short-circuiting stdout's event.

While this is not happening in Linux or Windows, no documentated
evidence exists for the order of event rediness in the event loop.

The test should not assume that buffer is consumed first by stdout.
@Fishrock123 Fishrock123 added the test Issues and PRs related to the tests. label Nov 26, 2015
@Trott
Copy link
Member

Trott commented Nov 26, 2015

Looks like this might benefit from a rebase against current master? I couldn't start CI successfully with this PR.

This change looks good to me assuming CI is OK and someone with some libuv expertise gives it a stamp of approval.

@mscdex mscdex added the child_process Issues and PRs related to the child_process subsystem. label Nov 26, 2015
@cjihrig
Copy link
Contributor

cjihrig commented Nov 26, 2015

LGTM if it works. As @Trott mentioned, this needs a rebase. The part of the test you are modifying was moved to test/parallel/test-child-process-spawnsync-maxbuf.js in ac319c3

@gireeshpunathil
Copy link
Member Author

@Trott , @cjihrig , thanks.

It was my mistake. Between the problem observation in AIX and my debugging and coming up with a fix, this file got bifurcated into two (test-child-process-spawnsync-maxbuf.js) - possibly to address the same underlying issue. Moreover, the new file seem to have amendments to mitigate the problem in a different way - by removing the interplay between stdout and stderr.

My proposed patch will allow this interplay, but will make the right assertion based on the order reversal.

The tests at their current state will not cause the reported problem, and no patch is required - in which case I can cancel this PR.
If we want to bring the original scenario, I can rework the patch to fit the context of test-child-process-spawnsync-maxbuf.js

Please let me know how do you want to do it.

@Trott
Copy link
Member

Trott commented Nov 27, 2015

I think the current bifurcated test setup tests everything that the test was intended to, so I'm OK with just leaving it as is. But I don't feel strongly if someone else disagrees.

@gireeshpunathil
Copy link
Member Author

Thanks @Trott , let us wait for a while then.

@gireeshpunathil
Copy link
Member Author

No comments so far. Assume the current test case(s) solve the issue. Cancelling this PR. Thank you very much @Trott!

This was referenced Mar 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants