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

error "write EINVAL" #16141

Closed
wenlonghuo opened this issue Oct 11, 2017 · 13 comments
Closed

error "write EINVAL" #16141

wenlonghuo opened this issue Oct 11, 2017 · 13 comments
Labels
child_process Issues and PRs related to the child_process subsystem. question Issues that look for answers. windows Issues and PRs related to the Windows platform.

Comments

@wenlonghuo
Copy link

wenlonghuo commented Oct 11, 2017

  • Version: 8.5.0,8.6.0
  • Platform: windows10 64-bit 1703
  • Subsystem:

while using child_process/spawn for ipc mode, child process will emit an error. it didn't appear in v8.4.0 or ubuntu

D:\fe\other\project\errortest>node index
hahah
events.js:182
      throw er; // Unhandled 'error' event
      ^

Error: write EINVAL
    at _errnoException (util.js:1019:11)
    at process.target._send (internal/child_process.js:704:20)
    at process.target.send (internal/child_process.js:588:19)
    at Object.<anonymous> (D:\fe\other\project\errortest\child.js:2:9)
    at Module._compile (module.js:624:30)
    at Object.Module._extensions..js (module.js:635:10)
    at Module.load (module.js:545:32)
    at tryModuleLoad (module.js:508:12)
    at Function.Module._load (module.js:500:3)
    at Function.Module.runMain (module.js:665:10)

file index.js

'use strict'
const spawn = require('child_process').spawn
const path = require('path')

let dir = path.join(__dirname, 'child.js')
let server = spawn('node', [`"${dir}"`], {
  stdio: ['pipe', 'ipc', 'pipe'],
  shell: true,
})
server.on('message', msg => {
  console.log(msg)
})

server.stderr.pipe(process.stderr)

file child.js

'use strict'
process.send('hahah')
@bnoordhuis bnoordhuis added child_process Issues and PRs related to the child_process subsystem. question Issues that look for answers. labels Oct 11, 2017
@joyeecheung joyeecheung added the windows Issues and PRs related to the Windows platform. label Oct 11, 2017
@joyeecheung
Copy link
Member

Couldn't reproduce on MacOS with v8.6.0 either. This is likely a Windows-specific issue

@vsemozhetbyt
Copy link
Contributor

Can reproduce on Windows 7 x64 with Node.js 4.8.4, 6.11.3, 8.7.0, 9.0.0 (last Nightly of 2017-10-11)

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Oct 11, 2017

cc @nodejs/platform-windows

@RReverser
Copy link
Member

Windows 10 x64 - doesn't fail on 8.4.0, but throws on 8.6.0.

@bzoz
Copy link
Contributor

bzoz commented Oct 18, 2017

This was introduced by #14791. It does not look related at all though.

@bzoz
Copy link
Contributor

bzoz commented Oct 18, 2017

Moving the "instantiate eagerly" part to the end of startup() fixes this issue.

@bnoordhuis
Copy link
Member

@bzoz With v8.6.0 too? The code from #14791 was removed in v8.6.0 in commit 8ce0e9a.

Semi-related: #16194

@bzoz
Copy link
Contributor

bzoz commented Oct 18, 2017

It does reproduce on 8.7.0, 8ce0e9a and master

@bzoz
Copy link
Contributor

bzoz commented Oct 18, 2017

This does not reproduce on neither Win 2012 nor 2008. It also does not happen on Win10 build 14393

@seishun
Copy link
Contributor

seishun commented Oct 19, 2017

The issue seems to be here:

node/deps/uv/src/win/pipe.c

Lines 1450 to 1458 in 8485a7c

req->event_handle = CreateEvent(NULL, 0, 0, NULL);
if (!req->event_handle) {
uv_fatal_error(GetLastError(), "CreateEvent");
}
if (!RegisterWaitForSingleObject(&req->wait_handle,
req->u.io.overlapped.hEvent, post_completion_write_wait, (void*) req,
INFINITE, WT_EXECUTEINWAITTHREAD)) {
return GetLastError();
}

An event is created and assigned to req->event_handle, but then RegisterWaitForSingleObject is called with req->u.io.overlapped.hEvent instead which is always equal to zero due to memset in

memset(&req->u.io.overlapped, 0, sizeof(req->u.io.overlapped));
Unless I misunderstand something, this call to RegisterWaitForSingleObject will always fail.

@seishun
Copy link
Contributor

seishun commented Oct 19, 2017

But why is the UV_HANDLE_EMULATE_IOCP flag set? The CreateIoCompletionPort call on the IPC handle fails here:

node/deps/uv/src/win/pipe.c

Lines 303 to 308 in 8485a7c

if (CreateIoCompletionPort(pipeHandle,
loop->iocp,
(ULONG_PTR)handle,
0) == NULL) {
handle->flags |= UV_HANDLE_EMULATE_IOCP;
}

GetLastError() returns ERROR_INVALID_PARAMETER. Continuing investigation.

@seishun
Copy link
Contributor

seishun commented Oct 19, 2017

The reason this worked before #14791 is because _process.setupChannel() was called before NativeModule.require('console'). As a result, CreateIoCompletionPort succeeded for IPC but failed for stdout. But that's fine because the UV_HANDLE_EMULATE_IOCP flag is irrelevant for writing if UV_HANDLE_BLOCKING_WRITES is set.

Why does CreateIoCompletionPort fail when called on two different handles for the same fd? Who knows, might be a Windows bug. I couldn't find the Windows repo on GitHub so I can't submit an issue.

Moving _process.setupChannel() before setupGlobalConsole(); should fix the issue, but this seems very fragile. It's clear that libuv's handling of CreateIoCompletionPort failures is broken, but I'm not familiar enough with libuv to formulate an issue.

@seishun
Copy link
Contributor

seishun commented Oct 19, 2017

Here's a similar case that should fail with all Node.js versions:

index.js

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

let dir = path.join(__dirname, 'child.js')
let server = spawn(process.argv0, [`"${dir}"`], {
  stdio: ['ipc', 'pipe', 'pipe'],
  shell: true,
})

server.stderr.pipe(process.stderr)

child.js

process.stdin.on('data', console.log);

I'm not sure this is something that can be fixed on Node.js/libuv's side at all.

gibfahn pushed a commit that referenced this issue Oct 30, 2017
Initializing IOCP on the same fd twice can fail on Windows.
Consequently, if the IPC channel uses fd 1 or 2 and the console is setup
first, writing to the IPC channel will fail.

PR-URL: #16562
Fixes: #16141
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this issue Oct 30, 2017
Initializing IOCP on the same fd twice can fail on Windows.
Consequently, if the IPC channel uses fd 1 or 2 and the console is setup
first, writing to the IPC channel will fail.

PR-URL: #16562
Fixes: #16141
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this issue Oct 31, 2017
Initializing IOCP on the same fd twice can fail on Windows.
Consequently, if the IPC channel uses fd 1 or 2 and the console is setup
first, writing to the IPC channel will fail.

PR-URL: #16562
Fixes: #16141
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this issue Nov 2, 2017
Initializing IOCP on the same fd twice can fail on Windows.
Consequently, if the IPC channel uses fd 1 or 2 and the console is setup
first, writing to the IPC channel will fail.

PR-URL: nodejs/node#16562
Fixes: nodejs/node#16141
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this issue Nov 2, 2017
Initializing IOCP on the same fd twice can fail on Windows.
Consequently, if the IPC channel uses fd 1 or 2 and the console is setup
first, writing to the IPC channel will fail.

PR-URL: nodejs/node#16562
Fixes: nodejs/node#16141
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Nov 15, 2017
It's not wholly clear what commit introduced the regression but between
v8.4.0 and v8.5.0 the 'resize' event stopped getting emitted when the
tty was resized.

The SIGWINCH event listener apparently was being installed before the
support code for `process.on('SIGWINCH', ...)` was.  Fix that by moving
said support code to real early in the bootstrap process.

This commit also seems to fix a Windows-only "write EINVAL" error for
reasons even less well-understood...

Fixes: nodejs#16141
Fixes: nodejs#16194
PR-URL: nodejs#16225
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
addaleax pushed a commit to ayojs/ayo that referenced this issue Dec 7, 2017
Initializing IOCP on the same fd twice can fail on Windows.
Consequently, if the IPC channel uses fd 1 or 2 and the console is setup
first, writing to the IPC channel will fail.

PR-URL: nodejs/node#16562
Fixes: nodejs/node#16141
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Dec 11, 2017
It's not wholly clear what commit introduced the regression but between
v8.4.0 and v8.5.0 the 'resize' event stopped getting emitted when the
tty was resized.

The SIGWINCH event listener apparently was being installed before the
support code for `process.on('SIGWINCH', ...)` was.  Fix that by moving
said support code to real early in the bootstrap process.

This commit also seems to fix a Windows-only "write EINVAL" error for
reasons even less well-understood...

Fixes: #16141
Fixes: #16194
PR-URL: #16225
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
gibfahn pushed a commit that referenced this issue Dec 20, 2017
It's not wholly clear what commit introduced the regression but between
v8.4.0 and v8.5.0 the 'resize' event stopped getting emitted when the
tty was resized.

The SIGWINCH event listener apparently was being installed before the
support code for `process.on('SIGWINCH', ...)` was.  Fix that by moving
said support code to real early in the bootstrap process.

This commit also seems to fix a Windows-only "write EINVAL" error for
reasons even less well-understood...

Fixes: #16141
Fixes: #16194
PR-URL: #16225
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
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. question Issues that look for answers. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants