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

Allow the code to decide if it should use read or read2. #3674

Closed
wants to merge 1 commit into from
Closed

Allow the code to decide if it should use read or read2. #3674

wants to merge 1 commit into from

Conversation

VanCoding
Copy link

This change makes the whole thing a bit more dynamic & it allows one to receive handles from pipes that we got from a pipe server. Hope you pull it.

bool ipc_pipe = wrap->stream_->type == UV_NAMED_PIPE &&
((uv_pipe_t*)wrap->stream_)->ipc;
bool ipc_pipe = (args.Length()>0)?(args[0].IsTrue()):(wrap->stream_->type == UV_NAMED_PIPE &&
((uv_pipe_t*)wrap->stream_)->ipc);
Copy link
Member

Choose a reason for hiding this comment

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

One, this is unreadable. Two, what do you think happens when you call uv_read2() on a stream type that doesn't support it?

Copy link
Author

Choose a reason for hiding this comment

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

I can of course make it more readable & handle errors, too. But only if you are willing to pull it then ;)

@piscisaureus
Copy link

it allows one to receive handles from pipes that we got from a pipe server

That will not work. Maybe on Unix.

@VanCoding
Copy link
Author

On unix, it definitely works, but I don't know about Windows. I'll check that ;)

@VanCoding
Copy link
Author

You're right @piscisaureus, it doesn't work on windows. But it works well on linux.

What's the problem on windows? Is windows not able to do it or is it just not implemented at the moment? Since handle passing over pipes works in the cluster module, it should also work with named pipes, doesn't it?

Anyway, I tried to do this as a simple workaround for a bug in libuv. Libuv doesn't set the "ipc" flag for pipe handles gotten from a pipe server handle. This results in the problem, that node also doesn't call read2 on them and its not possible to pass handles over such pipes then.

I've already asked to fix this in the following issue: joyent/libuv#490

If you plan to fix this soon anyway, then this pull request will become useless.

@VanCoding
Copy link
Author

As a proof that it works, you can look at the latest version of my module "node-ancillary" at https://github.com/VanCoding/node-ancillary. It has a native funciton "makeIPC" which does nothing more that setting the ipc property of uv_pipe_t handles to 1. This allows the whole module to do its job.

I hope there will be a way to do the same on windows soon.

@piscisaureus
Copy link

@VanCoding On windows there is no ancillary data for pipes. So IPC channels implement their own special protocol that only libuv understands. The thing is, there is no way to libuv to tell whether a pipe uses this IPC protocol, or is just "raw". When node spawns a child process with fork() it sets some environment variable so the child node knows which stdio handle uses the IPC protocol. For pipe servers there's not really a way to do this.

@VanCoding
Copy link
Author

Ok, good to know. But just to be sure: Since you use a protocol to exchange handles, it should theoretically work with every binary stream. It would even work with tcp sockets, right?

The only thing I'd have to do is to tell libuv that it shall use this ipc protocol for the stream. The only problem at the moment is that libuv doesn't know if it should use this protocol or just the raw stream for pipes it got from pipe servers.

If this is true, then I should still be able to manually make such a pipe use the ipc protocol. But how can I do it?

One more thing: No matther if it's a normal pipe or a pipe server, we can set the "ipc" property for both of them. Why don't you automatically use the ipc protocol for all pipes gotten from a pipe server whose ipc-property is set to true? It would make a lot of sense in my opinion. Again a linkt to my feature request at libuv: joyent/libuv#490

@Nodejs-Jenkins
Copy link

Can one of the admins verify this patch?

@chrisdickinson
Copy link

Closing this due to inactivity.

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.

None yet

5 participants