Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

stdio: Fix issue #3584. Use synch write in pipe #5595

Closed
wants to merge 1 commit into from
Closed

stdio: Fix issue #3584. Use synch write in pipe #5595

wants to merge 1 commit into from

Conversation

HenryRawas
Copy link

When pipe used for stdout or stderr on Windows,
pipe does not allow IOCP. In this case do write
on calling thread.
Added test test-child-process-stdout-flush-exit

@trevnorris trevnorris mentioned this pull request May 29, 2013
@trevnorris
Copy link

@HenryRawas in the future if there's anything you need to correct in the PR, go ahead and make the change then use git commit --amend then git push -f origin issue3584. GitHub will automatically pick up the change and they'll show up here.

if (handle->write_reqs_pending == 0) {
uv_queue_non_overlapped_write(handle);
}
DWORD bytes;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use two space indent. Changes to libuv should be submitted as a separate pull request to https://github.com/joyent/libuv (and preferably come with a test, though that may be somewhat difficult here.)

@piscisaureus
Copy link

Agree on that this needs to be fixed in node, but very much disagree with the implementation. This makes all pipe writes synchronous, not just the ones that we use for stdio.

@piscisaureus
Copy link

BTW hello @HenryRawas what have you been up to? :)

@HenryRawas
Copy link
Author

I don't agree that all pipe writes are synchronous - only ones with mode FILE_SYNCHRONOUS_IO. I think pipes opened by libuv will not have this set.

Hello Bert. I have done work on Redis and some other stuff for MsOpenTech.

@HenryRawas
Copy link
Author

After discussing with Bert I updated the pull request so that the pipe uses blocking writes only if the caller asks for it.
The fix is now split into 2. There is a libuv PR to change the pipe code, and a node PR to set the blocking flag for the pipe on stdout.

When pipe used for stdout or stderr on Windows,
pipe does not allow IOCP. In this case do write
on calling thread.
Added test test-child-process-stdout-flush-exit
@tjfontaine
Copy link

fixed in a different way on master

@tjfontaine tjfontaine closed this Mar 10, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants