-
Notifications
You must be signed in to change notification settings - Fork 155
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
Conversation
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. |
tACK on win32 with explicit decrediton request to close |
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 |
So dcrwallet would be using two pipes, one anonymous and the other a named pipe? |
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. |
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. |
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. |
There was a problem hiding this 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"` |
There was a problem hiding this comment.
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).
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 |
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)? |
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. |
There are some fundamental differences between a pipe and a socket that we'll have to consider here.
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 |
Correction: unix named pipes are unidirectional. |
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).
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.
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 :)). |
Just expanding on a bit from the previous answer:
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. |
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. |
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 |
Solved in a different way in decrediton so closing. May be discussed in the future. |
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.