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

Crash on windows when passing child process' stdin as other childs stdout #17493

Closed
mika-fischer opened this issue Dec 6, 2017 · 7 comments
Closed
Labels
child_process Issues and PRs related to the child_process subsystem. confirmed-bug Issues with confirmed bugs. windows Issues and PRs related to the Windows platform.

Comments

@mika-fischer
Copy link
Contributor

  • Version: v8.9.2
  • Platform: Windows 10 64bits

The following simple test crashes the node process on Windows. It works fine on Linux.

const {spawn} = require('child_process');

if (process.argv.length <= 2) {
    // parent
    const child2 = spawn(process.argv0, [process.argv[1], '.'], {
        stdio: ['pipe', 'ignore', 'ignore'],
    });
    const child1 = spawn(process.argv0, [process.argv[1], '.'], {
        stdio: ['pipe', child2.stdin, 'ignore'],
    });
    setInterval(() => child1.stdin.write(`.`), 1);
} else {
    // children
    process.stdin.pipe(process.stdout);
}

The error is:

Exception thrown: write access violation.
req was 0x2275CA37430.

It occurs in deps/uv/src/win/core.c:439.

Call stack:
callstack

Locals:
locals

I got the stack traces from a self-compiled build, but it also crashes with the official binary.

@mscdex mscdex added child_process Issues and PRs related to the child_process subsystem. windows Issues and PRs related to the Windows platform. labels Dec 7, 2017
@bzoz
Copy link
Contributor

bzoz commented Dec 11, 2017

@mika-fischer I can't reproduce this. I'm trying this with Node 8.9.2 (but also with 8.6.0 and latest nightly) on 64-bit Win 10 Pro and it works. Are you maybe redirecting the script output somewhere or using some other terminal (like ConEmu)?

@mika-fischer
Copy link
Contributor Author

I've tried it in plain cmd, cmder, VS2015 x64 Native Command Prompt and cmd inside VSCode. And of course it's best to run it in the debugger. In all those cases it always crashes for me.

So if you run it in the debugger with devenv /debugexe apps\node\Debug\node.exe tests\nodejs-process-pipe\bug.js it just continues running?

@bzoz
Copy link
Contributor

bzoz commented Dec 12, 2017

I did not test this correctly, it does indeed crash.

I've tested, this reproduces back to v6 (on v4 there is a TypeError when spawning child2).

I've made slightly modified version of your code which still reproduces:

const writeSize = 1

const spawn = require('child_process').spawn;

const who = process.argv.length <= 2 ? 'parent' : process.argv[2];


switch (who) {
    case 'parent':
        const consumer = spawn(process.argv0, [process.argv[1], 'consumer'], {
            stdio: ['pipe', 'ignore', 'inherit'],
        });
        const producer = spawn(process.argv0, [process.argv[1], 'producer'], {
            stdio: ['pipe', consumer.stdin, 'inherit'],
        });
        process.stdin.on('data', () => {})
        break
    case 'producer':
        const buffer = Buffer.alloc(writeSize, '.');
        const write = () => {
            process.stdout.write(buffer, write);
        }
        write()
        break
    case 'consumer':
        var totalDots = 0;
        process.stdin.on('data', (data) => {
            totalDots += data.length
            console.error(`Got ${data.length} dots! Total: ${totalDots}`)
        })
        break
}

The consumer will receive data from the producer so that part works. But then, it will crash at random point, between 600 and 900 bytes written.

If you change writeSize to about 1000, it will crash either:

  • instantly
  • after one write
  • after two writes, where the second one is much bigger:
    image

This looks like a race inside libuv, I'll investigate this further.

@bzoz bzoz added the confirmed-bug Issues with confirmed bugs. label Dec 12, 2017
@seishun
Copy link
Contributor

seishun commented Feb 2, 2018

Can't reproduce on master. It was probably fixed in #18260.

@bzoz
Copy link
Contributor

bzoz commented Feb 5, 2018

It was fixed in #18019. Maybe we should add this as a test to Node.js?

@cjihrig
Copy link
Contributor

cjihrig commented Feb 5, 2018

@bzoz a regression test is a good idea.

@bzoz
Copy link
Contributor

bzoz commented Feb 7, 2018

Test in #18614

@bzoz bzoz closed this as completed Feb 7, 2018
bzoz added a commit to JaneaSystems/node that referenced this issue Feb 12, 2018
Add two regression tests for stdio over pipes.

test-stdio-pipe-access tests if accessing stdio pipe that is being read
by another process does not deadlocks Node.js. This was reported in
nodejs#10836 and was fixed in v8.3.0.
The deadlock would happen intermittently, so we run the test 5 times.

test-stdio-pipe-redirect tests if redirecting one child process stdin to
another process stdout does not crash Node as reported in
nodejs#17493. It was fixed in
nodejs#18019.
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Feb 17, 2018
Add two regression tests for stdio over pipes.

test-stdio-pipe-access tests if accessing stdio pipe that is being read
by another process does not deadlocks Node.js. This was reported in
nodejs#10836 and was fixed in v8.3.0.
The deadlock would happen intermittently, so we run the test 5 times.

test-stdio-pipe-redirect tests if redirecting one child process stdin to
another process stdout does not crash Node as reported in
nodejs#17493. It was fixed in
nodejs#18019.

PR-URL: nodejs#18614
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this issue Feb 21, 2018
Add two regression tests for stdio over pipes.

test-stdio-pipe-access tests if accessing stdio pipe that is being read
by another process does not deadlocks Node.js. This was reported in
#10836 and was fixed in v8.3.0.
The deadlock would happen intermittently, so we run the test 5 times.

test-stdio-pipe-redirect tests if redirecting one child process stdin to
another process stdout does not crash Node as reported in
#17493. It was fixed in
#18019.

PR-URL: #18614
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
yhwang pushed a commit to yhwang/node that referenced this issue Feb 22, 2018
Add two regression tests for stdio over pipes.

test-stdio-pipe-access tests if accessing stdio pipe that is being read
by another process does not deadlocks Node.js. This was reported in
nodejs#10836 and was fixed in v8.3.0.
The deadlock would happen intermittently, so we run the test 5 times.

test-stdio-pipe-redirect tests if redirecting one child process stdin to
another process stdout does not crash Node as reported in
nodejs#17493. It was fixed in
nodejs#18019.

PR-URL: nodejs#18614
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit that referenced this issue Feb 26, 2018
Add two regression tests for stdio over pipes.

test-stdio-pipe-access tests if accessing stdio pipe that is being read
by another process does not deadlocks Node.js. This was reported in
#10836 and was fixed in v8.3.0.
The deadlock would happen intermittently, so we run the test 5 times.

test-stdio-pipe-redirect tests if redirecting one child process stdin to
another process stdout does not crash Node as reported in
#17493. It was fixed in
#18019.

PR-URL: #18614
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit that referenced this issue Feb 26, 2018
Add two regression tests for stdio over pipes.

test-stdio-pipe-access tests if accessing stdio pipe that is being read
by another process does not deadlocks Node.js. This was reported in
#10836 and was fixed in v8.3.0.
The deadlock would happen intermittently, so we run the test 5 times.

test-stdio-pipe-redirect tests if redirecting one child process stdin to
another process stdout does not crash Node as reported in
#17493. It was fixed in
#18019.

PR-URL: #18614
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this issue Feb 26, 2018
Add two regression tests for stdio over pipes.

test-stdio-pipe-access tests if accessing stdio pipe that is being read
by another process does not deadlocks Node.js. This was reported in
#10836 and was fixed in v8.3.0.
The deadlock would happen intermittently, so we run the test 5 times.

test-stdio-pipe-redirect tests if redirecting one child process stdin to
another process stdout does not crash Node as reported in
#17493. It was fixed in
#18019.

PR-URL: #18614
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
Add two regression tests for stdio over pipes.

test-stdio-pipe-access tests if accessing stdio pipe that is being read
by another process does not deadlocks Node.js. This was reported in
nodejs#10836 and was fixed in v8.3.0.
The deadlock would happen intermittently, so we run the test 5 times.

test-stdio-pipe-redirect tests if redirecting one child process stdin to
another process stdout does not crash Node as reported in
nodejs#17493. It was fixed in
nodejs#18019.

PR-URL: nodejs#18614
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 7, 2018
Add two regression tests for stdio over pipes.

test-stdio-pipe-access tests if accessing stdio pipe that is being read
by another process does not deadlocks Node.js. This was reported in
#10836 and was fixed in v8.3.0.
The deadlock would happen intermittently, so we run the test 5 times.

test-stdio-pipe-redirect tests if redirecting one child process stdin to
another process stdout does not crash Node as reported in
#17493. It was fixed in
#18019.

PR-URL: #18614
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 7, 2018
Add two regression tests for stdio over pipes.

test-stdio-pipe-access tests if accessing stdio pipe that is being read
by another process does not deadlocks Node.js. This was reported in
#10836 and was fixed in v8.3.0.
The deadlock would happen intermittently, so we run the test 5 times.

test-stdio-pipe-redirect tests if redirecting one child process stdin to
another process stdout does not crash Node as reported in
#17493. It was fixed in
#18019.

PR-URL: #18614
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 9, 2018
Add two regression tests for stdio over pipes.

test-stdio-pipe-access tests if accessing stdio pipe that is being read
by another process does not deadlocks Node.js. This was reported in
#10836 and was fixed in v8.3.0.
The deadlock would happen intermittently, so we run the test 5 times.

test-stdio-pipe-redirect tests if redirecting one child process stdin to
another process stdout does not crash Node as reported in
#17493. It was fixed in
#18019.

PR-URL: #18614
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 16, 2018
Add two regression tests for stdio over pipes.

test-stdio-pipe-access tests if accessing stdio pipe that is being read
by another process does not deadlocks Node.js. This was reported in
#10836 and was fixed in v8.3.0.
The deadlock would happen intermittently, so we run the test 5 times.

test-stdio-pipe-redirect tests if redirecting one child process stdin to
another process stdout does not crash Node as reported in
#17493. It was fixed in
#18019.

PR-URL: #18614
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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. confirmed-bug Issues with confirmed bugs. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

No branches or pull requests

5 participants