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

Creation of full duplex pipe via spawn causes deadlock on Windows #29238

Closed
haiyangwu opened this issue Aug 21, 2019 · 2 comments
Closed

Creation of full duplex pipe via spawn causes deadlock on Windows #29238

haiyangwu opened this issue Aug 21, 2019 · 2 comments
Labels
child_process Issues and PRs related to the child_process subsystem. windows Issues and PRs related to the Windows platform.

Comments

@haiyangwu
Copy link

haiyangwu commented Aug 21, 2019

  • v12.8.1:
  • 64-bit Windows 10:
  • Subsystem:

Recently tried to add windows support for mediasoup, https://github.com/haiyangwu/mediasoup-win, but found
Related Bug in LibUV, libuv/libuv#95,

Parent Process, node js:

    const spawnArgs = [ id ].concat(parameters);
    const spawnOptions =
    {
        detached : false,

        /*
            * fd 0 (stdin)   : Just ignore it.
            * fd 1 (stdout)  : Pipe it for 3rd libraries in the worker.
            *                  that log their own stuff.
            * fd 2 (stderr)  : Same as stdout.
            * fd 3 (channel) : Channel fd.
            */
        stdio : [ 'ignore', 'pipe', 'pipe', 'pipe' ]
    };

    // Closed flag.
    this._closed = false;

    // Create the mediasoup-worker child process.
    this._child = spawn(workerPath, spawnArgs, spawnOptions);

    // Channel instance.
    this._channel = new Channel(this._child.stdio[CHANNEL_FD]);
class Channel extends EnhancedEventEmitter
{
	constructor(socket)
	{
        // Read Channel responses/notifications from the worker.
		this._socket.on('data', (buffer) =>
		{
            // ....
        }
    }
}

Child Process, c++ :

        int err;
	this->uvHandle       = new uv_pipe_t;
	this->uvHandle->data = (void*)this;

	err = uv_pipe_init(DepLibUV::GetLoop(), this->uvHandle, 0);
	if (err != 0)
	{
		delete this->uvHandle;
		this->uvHandle = nullptr;

		MS_THROW_ERROR_STD("uv_pipe_init() failed: %s", uv_strerror(err));
	}

	err = uv_pipe_open(this->uvHandle, fd);
	if (err != 0)
	{
		uv_close(reinterpret_cast<uv_handle_t*>(this->uvHandle), static_cast<uv_close_cb>(onClose));

		MS_THROW_ERROR_STD("uv_pipe_open() failed: %s", uv_strerror(err));
	}

	// Start reading.
	err = uv_read_start(
	  reinterpret_cast<uv_stream_t*>(this->uvHandle),
	  static_cast<uv_alloc_cb>(onAlloc),
	  static_cast<uv_read_cb>(onRead));
	if (err != 0)
	{
		uv_close(reinterpret_cast<uv_handle_t*>(this->uvHandle), static_cast<uv_close_cb>(onClose));

		MS_THROW_ERROR_STD("uv_read_start() failed: %s", uv_strerror(err));
	}

This works like a champ on Linux/OSX, however, on Windows the payload from the child sometimes not gets sent.

already tried to change code in src/process_wrap.cc, and after test, my app works

    // ....
    } else if (type->StrictEquals(env->pipe_string())) {
#ifdef _WIN32
        options->stdio[i].flags =
            static_cast<uv_stdio_flags>(UV_CREATE_PIPE | UV_READABLE_PIPE |
                                        UV_WRITABLE_PIPE | UV_OVERLAPPED_PIPE);
#else
        options->stdio[i].flags = static_cast<uv_stdio_flags>(
            UV_CREATE_PIPE | UV_READABLE_PIPE | UV_WRITABLE_PIPE);
#endif
    } else if (type->StrictEquals(env->wrap_string())) {
    // ....
@Fishrock123 Fishrock123 added windows Issues and PRs related to the Windows platform. child_process Issues and PRs related to the child_process subsystem. labels Aug 22, 2019
@saghul
Copy link
Member

saghul commented Aug 29, 2019

Since UV_OVERLAPPED_PIPE is a noop on Unix, I'd suggest you set it unconditionally.

@jasnell
Copy link
Member

jasnell commented Jun 25, 2020

There's been no further activity here and it's not clear there's anything actionable. Closing but can reopen if necessary

@jasnell jasnell closed this as completed Jun 25, 2020
tarruda added a commit to tarruda/node-1 that referenced this issue Dec 31, 2020
The 'overlapped' value sets the UV_OVERLAPPED_PIPE libuv flag in the
child process stdio.

Fixes: nodejs#29238
aduh95 pushed a commit that referenced this issue Jan 3, 2021
The 'overlapped' value sets the UV_OVERLAPPED_PIPE libuv flag in the
child process stdio.

Fixes: #29238

PR-URL: #29412
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this issue Jan 12, 2021
The 'overlapped' value sets the UV_OVERLAPPED_PIPE libuv flag in the
child process stdio.

Fixes: #29238

PR-URL: #29412
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Aug 8, 2021
The 'overlapped' value sets the UV_OVERLAPPED_PIPE libuv flag in the
child process stdio.

Fixes: #29238

PR-URL: #29412
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this issue Aug 12, 2021
The 'overlapped' value sets the UV_OVERLAPPED_PIPE libuv flag in the
child process stdio.

Fixes: #29238

PR-URL: #29412
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Aug 31, 2021
The 'overlapped' value sets the UV_OVERLAPPED_PIPE libuv flag in the
child process stdio.

Fixes: #29238

PR-URL: #29412
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
foxxyz pushed a commit to foxxyz/node that referenced this issue Oct 18, 2021
The 'overlapped' value sets the UV_OVERLAPPED_PIPE libuv flag in the
child process stdio.

Fixes: nodejs#29238

PR-URL: nodejs#29412
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.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. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants