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

ipc: add named pipe read end #1288

Closed
wants to merge 1 commit into from
Closed

Conversation

matheusd
Copy link
Member

Adds a new --namedpiperx config option which triggers the creation of a server (either a pipe or unix socket depending on the platform) that can be used by a controlling parent app to start the shutdown of the app.

This is mainly needed for decrediton running in windows, which is having trouble correctly controlling the wallet with the existing filedescriptor based piperx/tx.

This can be tested in linux (and possibly mac) by using a local netcat connection to the named unix socket.

In windows the easiest way to test is to use this alongside the decrediton PR which modifies the win32ipc module to use named pipes instead of file descriptors.

This currently only implements the reading part, but if this approach is sound, then I can either modify this PR or send a new PR with the writing part, which should allow us to to drop the ugly hack of reading for log messages to discover the grpc port that is currently implemented in windows.

@jrick
Copy link
Member

jrick commented Sep 14, 2018

I looked into this before, and I think the reason why I didn't reach for named pipes previously was that there was no way to tell in the child when the parent process hung up. By using an anonymous pipe, the inherited fd (or handle on windows) is closed when the parent process exits, which can be used to signal clean shutdown of dcrd and/or dcrwallet.

@alexlyp
Copy link
Member

alexlyp commented Sep 14, 2018

tACK on win32 with explicit decrediton request to close

@matheusd
Copy link
Member Author

By using an anonymous pipe, the inherited fd (or handle on windows) is closed when the parent process exits,

That's how it's working today, exactly. But we're adding the ability in decrediton to close the currently open wallet so that you can then open a different one without having to exit the application, and using CloseHandle() on the anonymous pipe wasn't working in the module (possibly due to that same situation we got when we tried to make --pipetx work in windows).

@jrick
Copy link
Member

jrick commented Sep 14, 2018

So dcrwallet would be using two pipes, one anonymous and the other a named pipe?

@matheusd
Copy link
Member Author

matheusd commented Sep 14, 2018

Decrediton's 1647 switches to using the named pipe for reading on windows (the writing pipe isn't used in windows because we couldn't make it actually work). Non-windows are currently only using the anonymous ones.

Discussing with @alexlyp we're considering moving to using named pipes in all platforms to avoid having to special case which style of comms to use.

@jrick
Copy link
Member

jrick commented Sep 14, 2018

My only hesitation with the named pipes is that it would be simpler if we could use AF_UNIX sockets everywhere instead. The latest windows 10 does support AF_UNIX, however I don't believe there is any alternative for earlier windows versions, other than going through the TCP/IP stack.

@matheusd
Copy link
Member Author

Agreed it would be better if everything could be unix sockets.

I tried performing a quick search on whether AF_UNIX had been backported, but no luck so far. Also, it seems the go listener doesn't support opening sockets with it in windows yet.

Copy link
Member

@jrick jrick left a comment

Choose a reason for hiding this comment

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

Will dcrwallet serve connections on this same pipe? It's not clear to me who is the listener and who is the client if we make this bidirectional.

@@ -156,6 +156,7 @@ type config struct {
// IPC options
PipeTx *uint `long:"pipetx" description:"File descriptor or handle of write end pipe to enable child -> parent process communication"`
PipeRx *uint `long:"piperx" description:"File descriptor or handle of read end pipe to enable parent -> child process communication"`
NamedPipeRx string `long:"namedpiperx" description:"Named pipe (in windows) or unix socket of read end pipe to enable wallet -> client process communication"`
Copy link
Member

Choose a reason for hiding this comment

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

sockets, fifos, etc. are bidirectional, so there's no need for a tx/rx distinction. The winio package emulates this using two named pipes. Should simply name this Pipe and let the implementation be platform specific (named pipes are not usually called that on unix).

@matheusd
Copy link
Member Author

My thought process was to follow the same pattern established in the piperx/pipetx pair, so the wallet would be listening in the rx and connecting in the tx, but if you're ok with making this bidirectional we can go either way.

Refactoring this to make it bidirection, it might make more sense to make the wallet connect somewhere specified via the pipe config, since then you don't risk someone trying to race whatever is instantiating the wallet.

@jrick
Copy link
Member

jrick commented Sep 26, 2018

Well currently I think the problem is that different platforms have not just different implementations but possibly different semantics. I don't have a windows dev environment available to test, but do the named pipes need to already exist (like a unix FIFO) or will it create the pipes as needed (like listening on a unix socket)?

@matheusd
Copy link
Member Author

If you create the listening end (like I did in the PR) then it will be created (or fail if it already exists). If you use the connect version, then it will connect to an existing one or fail if it doesn't exist.

That might be another reason to have the two named{r,t}x options.

@jrick
Copy link
Member

jrick commented Sep 26, 2018

There are some fundamental differences between a pipe and a socket that we'll have to consider here.

  1. A pipe allows communication between exactly two processes. The pipe might be unidirectional or bidirectional. Pipes can be anonymous (as we currently have), be implemented as regular files (unix FIFOs), or be named under their own namespace (windows named pipes).

  2. A socket allows communication between a single listener and several clients. If we are using sockets anywhere, we will have to designate one process to be the listener and the other to be the client. At the moment, I don't know which is which.

  3. Both sockets and unix FIFOs allow bidirectional message passing. I believe windows sockets do not, and the winio package creates two separate pipes to handle both directions transparently (and therefore the name you must pass as the argument is not really a pipe itself).

  4. The closest similar concept to a windows named pipe would be a unix FIFO, however these do not have the desired behavior of the pipe closing when one process exits (and I don't know if windows named pipes do either -- this was the reason I originally built the IPC system with anonymous inherited pipe fds). I don't know if windows named pipes are closed when the attached process exits.

It's because of all these differences that I suggested considering an AF_UNIX socket on windows, but the downsides have already been iterated (poor Go support, only available on recent win10 releases, etc.). I'm not sure the best solution here, and there may not be any good ones at all which provide the same semantics on all platforms.

Remind me why the anonymous pipes don't work again? :p

@jrick
Copy link
Member

jrick commented Sep 26, 2018

Correction: unix named pipes are unidirectional.

@matheusd
Copy link
Member Author

Remind me why the anonymous pipes don't work again? :p

Because on windows the file descriptors returned by the custom node module in decrediton and specified via piperx/pipetx don't actually get mapped to the same stream and are unusable. Don't know if you recall those tests we did trying all sorts of tricks to make pipetx work.

IIRC there was some bug filing somewhere specifying the root cause for these sorts of problems was that the dcrwallet process and the node module (/process) were linked against different msvcrt versions, which caused them to use different file descriptor tables (such that fd 4 on the node module was not the same fd 4 in dcrwallet). And we couldn't figure out which fd it would get mapped into before running the wallet (and therefore being unable to specify the fd via command line args).

I don't know if windows named pipes are closed when the attached process exits.

Don't remember if I performed this test, but I tried coding so that it would always explicitly close as either end was shutting down.

I'm not sure the best solution here, and there may not be any good ones at all which provide the same semantics on all platforms.

Totally understand this. Sucks to have to implement platform specific stuff... :(

I can accept allowing this to be used only in windows (though I'd guess you wound't :)).

@matheusd
Copy link
Member Author

Just expanding on a bit from the previous answer:

[...] get mapped to the same stream and are unusable

Unusable here in the sense that we wanna be able to close the stream without the node process going away, as we want to be able to command the wallet to shutdown so that we can open it again.

@jrick
Copy link
Member

jrick commented Sep 27, 2018

Another issue I see happening with the named pipes is this will make it impossible to run multiple copies of decrediton (either as the same or a different user) since the pipe names will conflict. This could be an issue for unix sockets as well if the same user tried to start multiple decrediton instances.

@matheusd
Copy link
Member Author

Another issue I see happening with the named pipes is this will make it impossible to run multiple copies of decrediton (either as the same or a different user) since the pipe names will conflict

This particular one isn't an issue the way I did it: decrediton specifies the name of the pipe and it ensures it's unique before passing that to dcrwallet via namedpiperx (basically, it's a prefix + current time in ms - unlikely to be an issue unless someone is actively trying to race the connection).

@matheusd
Copy link
Member Author

Solved in a different way in decrediton so closing. May be discussed in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants