test: share maxBuffer between stdout and stderr #4035
Conversation
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.
|
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
added
the
child_process
|
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. Please let me know how do you want to do it. |
|
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. |
|
Thanks @Trott , let us wait for a while then. |
|
No comments so far. Assume the current test case(s) solve the issue. Cancelling this PR. Thank you very much @Trott! |
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.