Simultaneous calls to the same action can cause early returns/continues with the wrong results

[Mr0grog]

Originally found as a byproduct of #465, also noted over in the discussion on #491.

If two queues on the same Nightmare instance are operating simultaneously and both call the same action (e.g. evaluate()), it is possible for one call to pick up the results from the first call and finish early, continuing its cue with the wrong data.

Here's an example from #465:

var Nightmare = require('nightmare');
var page = Nightmare({show:true}).goto('http://yahoo.com');

page
  .wait(function() {
    return false;
  })
  .run(function(error, result) {
    if (error) { console.log("Error waiting for nothing:", error); }
    else { console.log("Inifnite wait finished, but shouldn't have! (uhoh)"); }
  });

page
  .wait(function() {
    return document.querySelector(".darla").firstChild.nodeName === "IFRAME";
  })
  .run(function(error, result) {
    if (error) { console.log("Error waiting for ad to load:", error); }
    else { console.log("First ad is loaded"); }
  });

// Prints:
// First ad is loaded
// Inifnite wait finished, but shouldn't have! (uhoh)

Or with typing:

var Nightmare = require('nightmare');
var page = Nightmare({show:true}).goto('http://yahoo.com').run(function() {
  page
    .type('#uh-search-box', 'github nightmare')
    .evaluate(function() {
      return document.querySelector('#uh-search-box').value;
    })
    .run(function(error, result) {
      if (error) { console.log("Error typing:", error); }
      else { console.log("Typed 'github nightmare'? " + result); }
    });

  page
    .type('#uh-search-box', 'ok')
    .evaluate(function() {
      return document.querySelector('#uh-search-box').value;
    })
    .run(function(error, result) {
      if (error) { console.log("Error typing:", error); }
      else { console.log("Typed 'ok'? " + result); }
    });
});

// Prints:
// Typed 'github nightmare'? goikt
// Typed 'ok'? goikt

Obviously there are more complicated questions with that one, but the salient bit here is that the queue typing "github nightmare" returned early. You can watch the window and see that it is still typing after the results are printed to the console.

You can do this with goto(), too, but that one's also somewhat complicated by the fact that goto() actually doesn't know how to fail (which is a different issue altogether). But it would fail to fail (ha!) if navigation were pre-empted by a second, simultaneous goto() call.

Obviously this is a little edge-case-y, but I suspect users will hit things like this more now that the "official" way to use Nightmare is via the then() API instead of with generators (where you have to go out of your way to trigger simultaneous actions).

There are a few potential ways I can think of to fix this.

1. Every cross-process call gets a unique ID. Replies would include that ID so the right listener handles it. (This could all be wrapped in up in a nice callback or promise based API, of course)
2. Ensure that multiple actions cannot be executed at once (two queues could be interleaved, but while queue A is executing an action, queue B would have to wait until that action is finished to execute its next action). This probably requires much more overhead to manage, but would fix issues like interleaved characters from two calls to type(). On the other hand, you might want most actions to be able to be simultaneous and type() might just be a little special.
3. Eliminate the possibility for multiple queues on a single nightmare instance altogether. Actions queued after a run() call would queue after run's callback (or possibly immediately before the callback, but after the actions it was running).
4. Something else? Those three are all I've got.

[Mr0grog]

It might also be worth noting that, while this is generally an edge-case, deep extensions of the sort that #425 aims to enable might be more prone to triggering these sorts of issues if they are slow and highly asynchronous.

[Mr0grog]

Some notes about thoughts I’ve had since I first posted this issue (or, an expansion of stuff from #561):

I think (1) or some flavor of it is essential. No matter what, it’s likely that some actions will have to do call multiple operations in the Electron process and those operations could collide. Even if everything in Nightmare’s core is extra careful, plugins could easy trip over things.

Given how often people run into problems with loops (#533, #526, #522 to name just a few recent ones) and how complex explaining the problem and various solutions is, I think (3) above or a variation on it is also a good idea. If all actions go onto a single, long-lived queue, even after the queue starts executing, that will completely eliminate looping issues.

As much as I like the idea of the power-user capability of being able to do multiple operations at once in the same window, I’m not sure there are a lot of concrete, real-world use cases for it. If there are good use-cases, I think they are sufficiently uncommon as to justify requiring a bit more work to enable them. Maybe an API like nightmareInstance.getQueue() would give you a new queue that you can add operations to (e.g. nightmareInstance.getQueue().click('#something').title().then(...)) and executes simultaneously with the “default” queue.

[rosshinkley]

I think (1) or some flavor of it is essential.

I agree. Having instances (whatever the final instance implementation is) be independent including the messaging bus is important.

Given how often people run into problems with loops and how complex explaining the problem and various solutions is, I think (3) above or a variation on it is also a good idea.

I tentatively agree. The question that immediately jumps to my mind is handling something like:

nightmare
  .goto(url)
  .someOtherAction()
  .evenMoreAction()
  .then(function(results){
     return nightmare.goto(anotherUrl)
       .yetAnotherAction()
       // etc
   })

I'm guessing you'd expect the first three actions to execute prior to the actions in .then(), right? Are you envisioning .then() being an action that acts like Underscore's/Lodash's .tap(), or would there be a differentiation between the queue being executed and the queue that is accruing tasks?

This could also lead to unintended consequences with memory for sufficiently large sets, but I'm okay (at least for the moment) with assuming that if you're using Nightmare to do an arbitrarily large number of tasks, you already Know What You're Doing™.

As much as I like the idea of the power-user capability of being able to do multiple operations at once in the same window, I’m not sure there are a lot of concrete, real-world use cases for it.

I would be inclined to support something closer to async's .parallel(). Something like:

nightmare
  .goto(someUrl)
  .parallel([
     nightmare.action().anotherAction(),
     nightmare.action().yetAnotherAction(),
     nightmare.anotherActionStill().somethingElse()
   ])
   .then(function(results){
     //results is an array of results.
     //could also support named hashes.
   });

Thoughts?

[Mr0grog]

Since we are in agreement that (1) should happen and since it resolves the critical safety issue, some notes on that first:

High-level goals:

1. Prevent two simultaneously performed actions from being able to mix up their results/errors
2. Be really easy to use (ideally, easier than the current method)
3. Support progress/streaming results? I think this would be neat, but I don’t think any existing Nightmare actions have need of it. Not sure how much of a priority to make it, but I suspect it would at least be wise not to implement a design that closes the door on it.

Proposed API:

// user process
this.child.send('command-name', arg1, arg2, ..., function(error, result) {
  // handle results
});

// electron process
parent.on('command-name', function(arg1, arg2, ..., done) {
  // do some stuff
  done(error, result);
});

This uses callbacks in order to be consistent and composable with Nightmare’s action API. That way, you can just pass the callbacks along:

actions.useragent = function(useragent, done) {
  debug('.useragent() to ' + useragent);
  this.child.send('useragent', done);
};

(If we wanted to change Nightmare’s API for actions to be promise-based, then I’d say this new API should be promises, too, but I don’t think anybody is pushing to do that).

On handling progress data, this.child.send() could be an event emitter (or a stream?) that emits anything the handler of parent.on('command') emits to its done argument. All events emitted there will only be received on the emitter created by calling this.child.send():

// user process
this.child.send('command-name', arg1, arg2, ..., function(error, result) {
    // handle results
  })
  .on('data', function() {
    // do something on progress
  });

// electron process
parent.on('command-name', function(arg1, arg2, ..., done) {
  done.emit('data', 'step 1');
  done.emit('data', 'step 2');
  ...
  done(error, result);
});

Internally, every call to this.child.send() would generate a unique ID (this could just be a incrementing an integer) and send a message named 'command' over IPC with the arguments: 'command-name', id, [command arguments]. Using the string 'command' as the event name allows us to safely differentiate between internal management commands and calls to actions.

On the electron side, a handler for 'command' events is responsible for creating a callback that will send a message back over IPC named 'command-result' with the arguments id, error, result.

Will post on queueing stuff in a bit.

[Mr0grog]

Heeeeeeere we go...

I'm guessing you'd expect the first three actions to execute prior to the actions in .then(), right?

Yes.

Are you envisioning .then() being an action that acts like Underscore's/Lodash's .tap(), or would there be a differentiation between the queue being executed and the queue that is accruing tasks?

I’m struggling to follow with what you’re getting at here and I may be about to go way off the rails (sorry if so). Slightly reconfigured:

nightmare
  .action1()
  .then(function then1 (results){
     return nightmare.action2();
  });

nightmare
  .action3()
  .then(function then2 (results) {...});

Are you asking whether action3 or action2 runs first? i.e. whether then() would be like pushing its onResolved handler onto the queue as an action, needing to complete before the next action (action3) runs? That is, is the resulting queue:

Time
 ↓    action1
 ↓    then1
 ↓      └─ action2
 ↓    action3
 ↓    then2

// or

 ↓    action1
 ↓       ├──── (schedules) ────┐
 ↓    action3                then1
 ↓       └──── (schedules) ────┼──────┐
 ↓    action2 ─────────────────┘    then2

At the very least, if then1 is expected to run before action3 (as opposed to simultaneously or after), I think that would mean action2 would have to run before action3, so it matches the basic behavior of promises. That is, if a promise’s onResolved/onRejected handlers return a then-able, they should not “complete” until the then-able they returned does. So the then1 action wouldn’t complete (enabling the queue to move on to action3) until the promise for the result of action2 completes.

That is to say, I do not think this would be acceptable because it either violates the expectation of promises/then or puts you in a deadlock (can't continue to action3 until action2 is done but action2 can't be performed until after action3):

Time
 ↓    action1
 ↓    then1 ── (queues) ─┐
 ↓    action3            │
 ↓    then2              │
 ↓    action2 ←──────────┘

(Except the above is what would theoretically happen if then1 didn’t return the nightmare instance (no more deadlock concern). But, mechanistically, it would be complicated to differentiate between the two. Hmmm.) Not to mention the question of what happens here:

nightmare
  .action1()
  .then(function then1 (results){
    return nightmare.action2();
  })
  .then(function then2 (results) {
    return nightmare.action4();
  });

nightmare
  .action3()
  .then(function then3 (results) {...});

So. I think I would go with the second flow of events above (with the very complicated chart) because the implementation seems very straightforward. Except. Complicated flow. :(

But then I want to imagine a magical land of fairies and unicorns where this works:

nightmare
  .action1()
  .then(function then1 (results){
     return nightmare.action2();
  })
  .action3()
  .then(function then2 (results) {...});

Which begs the question: is that different than the above code snippet? Reading it naively, it would definitely imply behavior like the first timing diagram above. You would really expect then1 to participate in the queue of actions and come before action3. Maybe it is:

 ↓    action1
 ↓       └── (schedules) ──┐
 ↓                       then1
 ↓    action2 ─────────────┘
 ↓       └── (schedules) ────────┐
 ↓                         (implicit then)
 ↓    action3 ───────────────────┘
 ↓       └── (schedules) ──────────────┐
 ↓                                   then2

I think (if I’m thinking right, but I’m getting a headache now) that emulates the behavior of the first timing chart but under the implementation that matches the second (very complicated timing chart). Or, under the first technical approach, the two code snippets function the same, while they function differently under the second.

How is that really different? Well... (deep breath)

nightmare
  .action1()
  .then(function then1 (results){
     return nightmare.action2(); // implicit then caused by return
  })
  .action3()
  .then(function then2 (results) {...});

nightmare
  .action4()
  .then(function then3 () {...});

Would be:

↓    action1
↓       ├──── (schedules) ────┐
↓    action4                then1
↓       └──── (schedules) ────┼──────┐
↓    action2 ─────────────────┘    then3
↓       └──── (schedules) ──────────────────┐
↓                                    (implicit then)
↓    action3 ───────────────────────────────┘
↓       └──── (schedules) ─────────────────────────┐
↓                                                then2

And this is starting to get crazy enough that I’m almost beginning to regret coming down this path. Sigh.

Or were you getting at what happens here?

nightmare
  .action1()
  .then(function then1 (results){
     return nightmare.action2();
  });

nightmare
  .action3()
  .action4()
  .then(function then2 (results) {...});

Being one of:

Time
 ↓    action1
 ↓    then1
 ↓      └─ action2
 ↓    action3
 ↓    action4
 ↓    then2

// or (actions invoked in `then` run ASAP but can’t interrupt another action)

 ↓    action1
 ↓       ├──── (schedules) ────┐
 ↓    action3 ────┐          then1
 ↓    action2 ────┼────────────┘
 ↓    action4 ────┘
 ↓       └──── (schedules) ────┐
 ↓                           then2

// or

 ↓    action1
 ↓       ├──── (schedules) ────┐
 ↓    action3                then1
 ↓    action4                  │
 ↓       └──── (schedules) ────┼──────┐
 ↓    action2 ─────────────────┘    then2

And I’m pretty sure I’ve gotten myself lost. Fresh eyes in the morning. Not going to get to explicit parallelism yet.

[rosshinkley]

process/execution safety

I agree with just about everything you said, including that this is a safety problem first and foremost. There are a few (many?) devils in the details, but I don't think they're worth touching until development is underway.

I really like the idea of progress events, especially for debugging. Implementing something like that would have made sussing out several bugs much easier. It also adds a new layer of power for actions/plugins.

I do have a question: how do you propose to have callbacks callable child->parent? I don't think you can call back like that, at least not without writing something reasonably fancy to manage the events for you. I feel like this is what you were getting at but never explicitly said it. I'm also feeling like maybe I've missed something important.

queues

And this is where the brain-bending really gets into full swing. It might be useful to explain what I was driving at: .then() would be an action, (almost) the same as anything else. It would let you put in custom code between other actions.

Skipping ahead a bit, that buys having the whole of the Nightmare API hung off of the .then() so you can get to your "magical land of fairies and unicorns". I'd add puppies and sunshine, but that's my two pence.

There are a couple of things with this approach that bother me. For one, I think the Nightmare instance doing the queueing would arguably need to be a full-on capital-P Promise implementation (whether that's done explicitly or with inheritiance). That would let it remain compatible with existing tools like the vo/co/mocha-generators family of libraries.

Second, and maybe more importantly, when does execution start if not with .then()? Is it the last .then() on a chain?

Moving on (well, backwards really), and borrowing your example here, and trying to clear up how this works in my brain:

nightmare
  .action1()
  .then(function then1 (results){
     return nightmare.action2();
  });

nightmare
  .action3()
  .then(function then2 (results) {...});

The resulting queue would look like:

action1
action3
then1
action2
then2
...

Which if I'm reading your diagrams properly, should match up with:

 ↓    action1
 ↓       ├──── (schedules) ────┐
 ↓    action3                then1
 ↓       └──── (schedules) ────┼──────┐
 ↓    action2 ─────────────────┘    then2

With respect to actions that return actions, I'd think that it wouldn't add an implicit then and instead alter the queue. Based on my above fun magical land commentary, and again borrowing your example:

nightmare
  .action1()
  .then(function then1 (results){
     return nightmare.action2(); // implicit then caused by return
  })
  .action3()
  .then(function then2 (results) {...});

nightmare
  .action4()
  .then(function then3 () {...});

(Aside: you're right, this gets nightmarish [har] in a hurry.)

I would expect this to look like:

action1
then1
action4
then3
action2
action3
then2
then3

... I think. I'm getting turned around trying to puzzle through this. Dearth of coffee.

And ultimately, I'm still head-tilting at arranging Nightmare calls in this way. It would run deterministically (unlike the wild of now), but I still don't think it would Do What You Mean™. A naive reading of the above makes it look like you're doing two completely independent chains of actions on a single nightmare instance, and based on what we're describing here, that's just not the case. (Nor should it be.)

With that said, now I'm starting to question what the ultimate goal is with the changes to queueing. I would advocate for 1) deterministic results and 2) easily usable/intuitive calls from .then(). Is that fair?

Re parallelism, yeah, that gets into a bit of a different bailiwick, but my two previous rules should still apply. I can think of ways to accomplish this, but I would like to see the queueing problem solved first. If I'm thinking about that properly, parallelism might fall very well fall out for cheap. Like you said, the implementation doesn't need to have it day zero, but I wouldn't want to completely close the door on it.

Oooookay. I've re-read your comments several times now, and my thinking parts are turning into mush. I know I've only scratched the surface here. I'll continue to let this marinate and comment again if I have more thoughts.

[Mr0grog]

There are a few (many?) devils in the details, but I don't think they're worth touching until development is underway.

BOOM: #579. Now we’ve got a separate place to discuss that one.

[GautierT]

After reading everything you said i still dont understand why i can't run multiple instance of new Nightmare() on the same machine.
If i launch the same script 5 or 6 times at the same time then some of them are going to hang (nothing happen or stay on nightmare:log altering page to force rendering) or crash (nightmare:log crashed [{},false])
Do you have some idea to help me debug that ? Thanks !

[Mr0grog]

@GautierT I think you might be a bit mixed up here—nothing on this issue is about problems running multiple instances of Nightmare at once (in fact doing so solves the issues here; this is about doing multiple sets of actions with one Nightmare instance).

Your problem sounds a lot like #612. One thing you might want to try is using random partition names or memory partitions, like so:

var nightmare = Nightmare({
  // Make sure that, not only are we using a memory partition, but that
  // it is probably a different one than any we've used before.
  webPreferences: {partition: 'custom-partition' + Math.random()}
});

If that doesn’t help, you might want to make a separate issue, since your problem is definitely off-topic for this thread.

[GautierT]

@Mr0grog : Ok. Sorry for the miss understanding.. !
My current partition setting is
webPreferences: { partition: 'nopersist' },
and i haven't set session.
I will try with a random partition number !

[Mr0grog]

@GautierT No worries! I hope you get it figured out.

[matthewmueller]

Hey guys, thanks for all the feedback here, but I'm going to close this one. Having 2 queues working on the same page in electron is race-y in a number of ways.

I'd like us to move towards what puppeteer does, where it creates a new page for each browser instance. Something like this:

const nightmare = await Nightmare()
const browser1 = await nightmare.page()
const browser2 = await nightmare.page()

Honestly when I first re-wrote nightmare to use electron – I messed this up badly. Sorry for the pain and time 😅