Callback-based IPC

[Mr0grog]

This addresses one aspect of #493. It should make IPC between the user’s process and the Electron process a) “safe” in that simultaneous calls can’t cause incorrect results and b) simpler. It’s based on discussion with @rosshinkley in issue #493.

The main idea here is that Nightmare’s IPC objects gain two methods, each to be used in opposite processes:

• ipc.respondTo(name, responder) Registers a function that can respond to calls from another process. The name is a handle by which other processes call it. The responder takes any number of arguments, where the last one is a callback. This callback should be called when the responder’s work is complete. Any arguments will be passed back to the caller in the other process. In addition, the callback has a method named progress that can be called to emit data events in the other process.

  ipc.respondTo('cookies.get', function(query, done) {
    webContents.session.cookies.get(query, function(error, cookies) {
      if (error) { return done(error); }
      done(null, query.name ? cookies[0] : cookies);
    });
  });

• ipc.call(name, [arg, [arg, [...]]], [callback]) Calls a responder with the given name in another process. Any arguments between the name and callback are passed as arguments to the responder. The callback will be called when the responder’s work is finished. In addition, this returns an event emitter than can be used to handle data events indicating progress from the responder or a single end event, which will be called with the same arguments as and immediately prior to callback.

  ipc.call('cookies.get', {name: 'my-cookie'}, function(error, cookie) {
    if (error) { return console.error('UHOH!', error); }
    console.log('OM NOM NOM I GOT ME A COOKIE!', cookie);
  });

Under the hood this is accomplished by having three special IPC messages:

• CALL indicates that a responder should be called. Sent from the parent, to the child. It has at least 2 arguments:
  1. A unique ID identifying the call. This is used to ensure responses don’t get tied up with other calls to the same responder.
  2. The name of the responder to call
  3. Arguments to pass to the responder.
• CALL_DATA_{ID} indicates progress data related to a call is being sent. Sent from the child to the parent. The ID in the name is the unique ID that was sent with the initial call above.
• CALL_RESULT_{ID} indicates that this is the final result of the call. Sent from the child to the parent. The ID in the name is the unique ID that was sent with the initial call above.

The above set of messages can be used in either direction, but they are probably only relevant now in terms of calls from the parent to the child. Maybe worth noting: a very similar (but more heavily built out and robust) mechanism is used internally in Electron’s remote module. This one is more minimal and light-weight, but sacrifices a lot of nice features that one has (support for circular references, complex types like dates, functions, promises, errors, and placeholder objects instead of string names).

Notes and open questions

1. This doesn’t convert all calls yet. It just does evaluate_now and useragent as demo cases. A later commit will have to do the rest.
2. In implementing this, I noticed that we can wind up with multiple IPC objects for a single process. It doesn’t actively cause problems, but depending on the order things are registered in, it could, so I made sure there could only be one IPC instance per process.
3. This implementation differs slightly from the initial proposal in Simultaneous calls to the same action can cause early returns/continues with the wrong results #493—it is intended to be backwards-compatible (so it doesn’t break plugins). I’m not sure if that a virtue or unnecessary, since plugins that can run in the Electron process are pretty new. The method names are also slightly different; I thought these were clearer.
4. The progress capability is neat, but it incurs a lot of additional complexity and I’m not sure how much real value it adds. No existing actions use anything like it. The situations where it would be useful might be just as well handled with more judicious debug messages, error throwing, or simple events. We could easily remove the progress support and add it again later.
   • If we keep it, should be a stream instead of a simple emitter? Like the progress capability itself, its benefits are primarily theoretical and it probably comes with a more complex implementation.
5. This implementation sits atop the existing ipc.emit() and ipc.on() methods. That means the special CALL* messages can be pretty easily observed. I’m not sure that’s a big deal, but we could call the appropriate methods on the process instead to make intercepting things harder.
6. My initial implementation relied heavily on the spread operator, which made things much simpler. However, spread isn’t available in Node v4.x without the --harmony flag (it’s 👍 in v5.x), so I removed it in favor of lots of apply() calls.
   • --harmony is still mentioned in the README (only down at the bottom), but not actually required in practice on Node 4+. I think it’s good that it’s not required.
   • In hindsight, a lot of the apply() noise (not all) could be avoided by just sending arguments as an array through IPC instead of spreading them into the message arguments.
7. Some action implementations in the Electron process will get more complex (only slightly) because of this. Several actions are fire-and-forget, but in this implementation, they have to call a callback. Because arguments are so dynamic in Nightmare, I can’t think of a good way to detect whether an action (on the Electron side) actually uses the callback. Probably not a big deal, but I just wanted to call it out.
8. You only get to have one responder for a given name. I think that’s reasonable (what would you do with more??), but not sure if it’s worth adding some logging or throw errors if respondTo is called with the same name more than once.
9. There’s no way to unregister a responder. I’m not sure we need it, but maybe?

I’d love feedback from anybody on this. Ideally this is a safe, backwards-compatible change (see (3) above), but it’s still a major (and deep) API change. We should be more 👍 than ¯_(ツ)_/¯ in order to accept it.

[rosshinkley]

The progress capability is neat, but it incurs a lot of additional complexity and I’m not sure how much real value it adds. No existing actions use anything like it. If we keep it, should be a stream instead of a simple emitter?

I'm mixed on this, but I can think of applications where this might be useful. For example, a similar concept is used in the download manager to look at download state out-of-band.

This implementation differs slightly from the initial proposal in #493—it is intended to be backwards-compatible (so it doesn’t break plugins). I’m not sure if that a virtue or unnecessary, since plugins that can run in the Electron process are pretty new.

My first off-the-cuff idea would be to wrap .on() and/or .emit() to warn of safety concerns when it is used in plugins (although I can think of situations where one might want to manage events themselves). What change were you planning to propose that would break plugins?

Also, while I have in interest in how plugins are implemented as I have authored a few, I don't think I'm the only author.

You only get to have one responder for a given name. I think that’s reasonable (what would you do with more??), but not sure if it’s worth adding some logging or throw errors if respondTo is called with the same name more than once.

I'm struggling to come up with a usecase for more than one responder. I would think responding to a given event twice would constitute a runtime error.

There’s no way to unregister a responder. I’m not sure we need it, but maybe?

Maybe. You could be explicit about unregistering a responder before registering a different responder for the same action, eg if you wanted your plugin implementation of .type() to override the default. (Overrides at present have no consequence, at least in my naive reading.)

More as I let this marinate and as others chime in. :)

[Mr0grog]

a similar concept is used in the download manager to look at download state out-of-band.

Oh, neat. I don’t think the progress mechanism here would simplify that use case, but it does make me wonder if there’s room for some sort of more abstract create-a-ipc-sub-channel mechanism. That would probably have some utility in the single-electron-process model described in #561. Probably out-of-scope here, though?

My first off-the-cuff idea would be to wrap .on() and/or .emit() to warn of safety concerns when it is used in plugins (although I can think of situations where one might want to manage events themselves).

I feel like on/emit still have a lot of value for sending events or event-like messages. As far as built-in stuff, I think that’s just event forwarding from the browser window and logging from the Electron process.

What change were you planning to propose that would break plugins?

I think I mistyped it on the issue thread, but I was originally thinking of replacing emit/on with call/respondTo instead of having differently-named methods.

I would think responding to a given event twice would constitute a runtime error.

So do you think it should throw an exception? Or maybe just log a warning?

[rosshinkley]

... but it does make me wonder if there’s room for some sort of more abstract create-a-ipc-sub-channel mechanism. That would probably have some utility in the single-electron-process model described in #561. Probably out-of-scope here, though?

I figured the single process model would have to have another identifier in .call()/.respondTo() (or .on()/.emit()) for which BrowserWindow the action was intended for. That could be pretty readily abstracted away for ease of use. Am I anywhere close to the same page?

That said, it's probably out of scope, but it does feel like process management/window management is going to depend on this change. (Or at least borrow from it heavily.)

I feel like on/emit still have a lot of value for sending events or event-like messages. As far as built-in stuff, I think that’s just event forwarding from the browser window and logging from the Electron process.

Fair, maybe a warning is too heavy-handed. I don't necessarily disagree, but it does seem like a raw .on()/.emit() pair is dangerous enough to warrant... something. Would this be better served as a (eventual) writeup on how to manage events yourself in nightmare-examples?

I think I mistyped it on the issue thread, but I was originally thinking of replacing emit/on with call/respondTo instead of having differently-named methods.

Ahhh, okay. I understand.

So do you think it should throw an exception? Or maybe just log a warning?

I want to say exception, but I'm not sure I can make a case. In my read of your implementation, I don't think you can cause execution problems as the results are executed with .once(), which makes me think warning. On the other side, I think that >1 response is errant behavior, shouldn't happen, and as such should be dealt with as an exception (if nothing else to catch silly stuff during testing, which is what's tipping me - but only just - toward exception). Is there a valid case where a .call() will have more than one response?

[Mr0grog]

it's probably out of scope, but it does feel like process management/window management is going to depend on this change. (Or at least borrow from it heavily.)

Definitely out of scope, but if call returned a scoped IPC instance instead of the emitter it does…

function Nightmare(options) {
  ...
  this.window = electronProcess.call('createWindow', function onEnded(reason) {
    // mark instance as cleaned up and done
  });
}

// in actions.js
exports.useragent = function(useragent, done) {
  this.window.call('useragent', useragent, done);
};

it does seem like a raw .on()/.emit() pair is dangerous enough to warrant... something.

At the very least, I should update the plugin example in the README. Totally forgot about that.

Maybe (this is a bit hare-brained), rename emit to send and make emit log a warning, then forward to send? That way, we are forcing people to opt-in to the fact that they don’t want the call-response structure. It’s kind of ridiculous to rename a method to make people consider whether they really want to use it, though.

Really I think it should be clear that this call is the better, future API and you can opt-in to improved behavior. emit/on isn’t getting any worse.

I don't think you can cause execution problems as the results are executed with .once()

True, but that kind of problem is actually prevented even earlier by the fact that the responder registry can only hold one responder per name. So registering a second one with the same name bumps out the old one, and there is only ever one function that even has the opportunity to call back. I think we established that that’s fine.

The “should we throw?” question is more about whether it’s ok to bump out a previously registered responder.

• Logging a warning would notify you that a responder was overridden, but would still allow your program to continue.
• Throwing would basically mean you can’t override a previous responder; doing so would just stop everything and Nightmare couldn’t run until you fixed the issue.
• Staying silent as the implementation currently does is probably not a great idea the more I think about it. You’d probably at least want some diagnostic logging informing you that the original responder has been replaced.

[Mr0grog]

I went ahead and added logging of duplicate calls to respondTo with the same name. Can easily change to throwing if we decide that’s preferable, though.

[rosshinkley]

@Mr0grog Good.

I'm considering opening a branch off of master for v3 parts. Would you be willing to complete this PR and put it against that branch? Thoughts on doing such a thing?

[Mr0grog]

Would you be willing to complete this PR and put it against that branch?

Sure? What’s “completing” mean—just wrapping up the rest of the built-in actions?

Thoughts on doing such a thing?

Generally I’d flip it around and have master be current development (e.g. v3) and have a v2 branch for any additional 2.x releases. But ¯_(ツ)_/¯ all good either way.

[rosshinkley]

Sure? What’s “completing” mean—just wrapping up the rest of the built-in actions?

Yeah, that's what I had in mind - rounding out the rest of the existing methods with the IPC safety is sufficient for now. (The beauty of source control: we can always change it later.)

Generally I’d flip it around and have master be current development (e.g. v3) and have a v2 branch for any additional 2.x releases.

I've seen this done a lot of different ways, and all seem to be about equally as effective. I had no real reason beyond releases being cut from the master branch and that PRs are probably going to continue to come to the master branch. There also seems to be artifacts of a historical precedence from the v2 stuff for Electron being done in a separate branch, but my research there is cursory at best.

I don't have a problem with changing that, even if it adds near-term work for existing PRs/merges.

But ¯_(ツ)_/¯ all good either way.

100% with you. I'm not compelled one way or another. If you've got a good reason, we can do it how you suggested. Or not. You know, whichever. :)

[Mr0grog]

@rosshinkley do you want a new PR pointing to another branch, or should this still be going to master? (per discussion the other day about whether this is v2.4 or v3 feature)

[rosshinkley]

I think we agreed that since this is non-breaking, this should go into 2.4.x. To that end, I think it should go into this PR => master.

[Mr0grog]

Cool, I think this is done if nothing looks out of place to you, then.