Adapt NodeIPC module from cardano-sl #388

rvl · 2019-06-10T07:35:04Z

Relates to issue #144

Overview

Takes the cardano-sl/cardano-shell NodeIPC code, cleans it up, and splits the general NodeJS child_process IPC protocol out from the Daedalus/Cardano specific protocol, improves the exception handling, changes the logging, and just makes it simpler.
Integrates DaedalusIPC into the server and launcher.
Adds an integration test which starts the launcher from nodejs, sending QueryPort, waiting for ReplyPort, as Daedalus does.
Adds a test for launcher --state-dir in the same Spec.

KtorZ · 2019-06-10T08:05:45Z

~~@rvl I am afraid you've done some double-work done on this PR :s ... ~~

~~Have you looked into cardano-shell (as mentioned in #144) -> https://github.com/input-output-hk/cardano-shell/tree/master/src/Cardano/Shell/NodeIPC ?~~

Nevermind, just saw your comment on your development plan:

it's not suitable for re-use because testing code is mixed up with message protocol code which is mixed up with the ipc code.

KtorZ

LGTM overall, but haven't really got time to play with it.

Also, some modification regarding ports have already been included in #407

KtorZ · 2019-06-13T16:48:56Z

lib/core/cardano-wallet-core.cabal

@@ -76,6 +78,7 @@ library
      Cardano.Wallet.Api
      Cardano.Wallet.Api.Server
      Cardano.Wallet.Api.Types
+      Cardano.Wallet.DaedalusIPC


Not a big fan of referring to Daedalus here. The IPC stuff can be seen as a low-level protocol to allow a client to perform some process management task of the wallet server. Of course, the primary user is Daedalus, but I don't want us to fall in the trap of "making stuff just for the frontend" as a Daedalus*** module would suggest. This already caused us quite some pain with the old API that has some non-sensical stuff into it.

In short: let's build more generic interfaces that serve a well-defined purpose.

I agree that NodeIPC was rather confusing, so maybe have something like NodeJSIPC or JSClientIPC

One of our main requirements is working with the Daedalus frontend. This ipc protocol is unique to Daedalus. I'm not aware of any requirements for integrating with some imaginary software that doesn't exist yet, but we can change the name if that happens.

It would be better to document the ways of starting cardano-wallet server on a port, so that prospective users can assess whether any of the schemes would work for them.

KtorZ · 2019-06-13T16:50:44Z

lib/core/src/Cardano/Wallet/DaedalusIPC.hs

+        sayErr "[INFO] Daedalus IPC is not enabled"
+        sleep
+    Left (NodeChannelBadFD err) ->
+        sayErr $ "[ERROR] Starting Daedalus IPC: " <> err


Now that we've the logging infrastructure setup, I'd suggest to switch to using the trace object instead of any other outputting method 👍

KtorZ · 2019-06-13T16:54:07Z

lib/http-bridge/test/integration/js/mock-daedalus.js

+      });
+    }
+  });
+}


Would be worth adding a negative test case with Daedalus sending some gibberish at first, to see that the ParseFailure message gets triggered correctly 👍

Added a test. You might have noticed the tests are a bit thin. That's because

cardano-shell has more tests, and we will probably move the code there

I think it's worth considering inheriting the socket from the parent process, rather than implementing some hokey message protocol.

KtorZ · 2019-06-13T16:57:31Z

lib/core/test/integration/Test/Integration/Scenario/CLI/Server.hs

+-- run launcher in the tests. But that dependency can't be expressed in the
+-- cabal file, because otherwise there would be a cycle.
+--
+-- So one hacky way to work around it is by running programs under "stack exec".


KtorZ · 2019-06-13T17:02:14Z

lib/core/src/Cardano/Wallet/DaedalusIPC.hs

+    -> IO ()
+ipcListener handle onMsg chan = do
+    hSetNewlineMode handle noNewlineTranslation
+    catch (race_ replyLoop sendLoop) onIOError


Nice pattern. I'd have a slight preference for concurrently instead of race_ since we aren't actually expecting any of the two sides to eventually terminate (which race_ conveys). We still get the cancellation behavior with concurrently if I am correct.

It really doesn't matter which one. Both of them imply the wrong thing. But if concurrently makes you happy, then I'll change it.

KtorZ · 2019-06-13T17:03:09Z

lib/core/src/Cardano/Wallet/DaedalusIPC.hs

+    onIOError :: IOError -> IO ()
+    onIOError err = do
+      sayErrString $ "[ERROR] Exception caught in DaedalusIPC: " <> show err
+      when (isEOFError err) $ sayErr "[DEBUG] it's an eof"


An EOF here would mean that Daedalus / the client has closed the connection on their hand, right ?

Usually the exception is usually broken pipe. I think this was debug code that got left in cardano-sl from the DEVOPS-1234 investigation. I have removed the log anyway.

KtorZ · 2019-06-13T17:09:54Z

lib/core/src/Cardano/Wallet/DaedalusIPC.hs

+        act
+    Left NodeChannelDisabled -> do
+        sayErr "[INFO] Daedalus IPC is not enabled"
+        sleep


Why sleeping here? What would be the consequence of returning () ? Do we expect daedalusIPC to never return? If so, I'd rather have it mentioned in a function comment to explain this behavior.

Yes, it should have been commented. This is the error handling I want:

If set up is unsuccessful, return immediately, causing the cardano-wallet process to exit. Daedalus does not want zombie cardano-wallet processes hanging around that it can't communicate with.

If set up is successful, never return until Daedalus exits.

If not running within a nodejs channel, never return (simplifies calling code)

Okay, so we really assume that any termination is an error, hence the sleep when there's no node.js channel (not an error so to speak).

KtorZ · 2019-06-13T17:12:28Z

exe/wallet/Main.hs

-    Server.start logStartup walletPort wallet
+    Server.withListeningSocket walletPort $ \(port, socket) -> do
+        let settings = Server.mkWarpSettings logStartup port
+        race_ (daedalusIPC port) (Server.startOnSocket settings socket wallet)


Ah! I supposed this is why we make the daedalusIPC hangs forever. I believe that switching for concurrently instead of race_ would remove that limitation since the action could return without cancelling the startOnSocket.

This needs to be race so that the program exits when there is an error from daedalusIPC.

Got you, this comment of yours clarifies the choice of race indeed. I'd suggest to have the behavior documented either on the call-site or, somewhere in the DaedalusIPC. Maybe in both places.

edit: I see it's now the case

This takes the cardano-sl/cardano-shell NodeIPC code and splits the general nodejs child_process IPC protocol out from the Daedalus/Cardano specific protocol, improves the exception handling, changes the logging, and just makes it simpler.

Removing a type constructor can change the shape of the JSON.

KtorZ · 2019-06-14T18:00:49Z

Merging into #414 to group PRs and avoid multiple merges because of CI issues due to stack major bump today ....

rvl added this to the Integrate node.js IPC listener in the launcher milestone Jun 10, 2019

rvl self-assigned this Jun 10, 2019

KtorZ mentioned this pull request Jun 10, 2019

Make the wallet into a child_process IPC server with support for QueryPort/ReplyPort #144

Closed

16 tasks

rvl force-pushed the rvl/144/daedalus-ipc branch from 6e79d65 to 5cdcab3 Compare June 11, 2019 06:58

rvl marked this pull request as ready for review June 11, 2019 07:08

Sam-Jeston mentioned this pull request Jun 12, 2019

[DDW-699] Add Cardano wallet build deps to nix input-output-hk/daedalus#1415

Closed

rvl force-pushed the rvl/144/daedalus-ipc branch 3 times, most recently from 9695881 to f7e12ed Compare June 13, 2019 05:54

KtorZ approved these changes Jun 13, 2019

View reviewed changes

rvl force-pushed the rvl/144/daedalus-ipc branch 3 times, most recently from e6cf806 to 3d63b7b Compare June 14, 2019 03:11

rvl and others added 12 commits June 14, 2019 18:43

Add DaedalusIPC module

2d4676b

This takes the cardano-sl/cardano-shell NodeIPC code and splits the general nodejs child_process IPC protocol out from the Daedalus/Cardano specific protocol, improves the exception handling, changes the logging, and just makes it simpler.

Integrate Daedalus IPC into CLI server start

2578b9c

tests: Add nodejs child_process spawner

f304606

tests: Add launcher --state-dir integration test

8fcef2f

tests: Add DaedalusIPC test

8df7e67

DaedalusIPC: Remove Ping/Pong messages

3e2434b

Regenerate nix

bfaf008

DaedalusIPC: Remove Cabal library dependency (yuck)

03f3b00

Fix rebase merge mistakes

51d375c

Address review comments

9ccfb53

DaedalusIPC: Don't use generic aeson encoding - too fragile

ebdd013

Removing a type constructor can change the shape of the JSON.

change 'cardano-wallet-launcher' to 'cardano-wallet launch'

5413101

KtorZ force-pushed the rvl/144/daedalus-ipc branch from 3d63b7b to 5413101 Compare June 14, 2019 18:00

KtorZ changed the base branch from master to KtorZ/368/merge-cardano-launcher June 14, 2019 18:00

KtorZ merged commit d9b3553 into KtorZ/368/merge-cardano-launcher Jun 14, 2019

KtorZ deleted the rvl/144/daedalus-ipc branch June 14, 2019 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapt NodeIPC module from cardano-sl #388

Adapt NodeIPC module from cardano-sl #388

rvl commented Jun 10, 2019 •

edited

Loading

KtorZ commented Jun 10, 2019 •

edited

Loading

KtorZ left a comment

KtorZ Jun 13, 2019

KtorZ Jun 13, 2019

rvl Jun 14, 2019

KtorZ Jun 13, 2019

rvl Jun 14, 2019

KtorZ Jun 13, 2019

rvl Jun 14, 2019

KtorZ Jun 13, 2019

KtorZ Jun 13, 2019

rvl Jun 14, 2019

KtorZ Jun 13, 2019

rvl Jun 14, 2019

KtorZ Jun 13, 2019

rvl Jun 14, 2019

KtorZ Jun 14, 2019

KtorZ Jun 13, 2019

rvl Jun 14, 2019

KtorZ Jun 14, 2019 •

edited

Loading

KtorZ commented Jun 14, 2019

Adapt NodeIPC module from cardano-sl #388

Adapt NodeIPC module from cardano-sl #388

Conversation

rvl commented Jun 10, 2019 • edited Loading

Overview

KtorZ commented Jun 10, 2019 • edited Loading

KtorZ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ Jun 14, 2019 • edited Loading

Choose a reason for hiding this comment

KtorZ commented Jun 14, 2019

rvl commented Jun 10, 2019 •

edited

Loading

KtorZ commented Jun 10, 2019 •

edited

Loading

KtorZ Jun 14, 2019 •

edited

Loading