async/await breaks the control flow

[sjelin]

Meta -

OS:
Ubuntu
Selenium Version:
2.53.3 or 3.0.0-beta-3
Browser:
node
Browser Version:
v5.10.1 or v4.0.0

Bug

I was trying to improve async/await support for Protractor when I ran into a problem with the folllowing test:

describe('async function + control flow', function() {
  var val;
  var seven;
  it('should wait for this it() to finish', async function() {
    val = 1;
    seven = await webdriver.promise.fulfilled(7);
    controlFlow.execute(() => {
      return webdriver.promise.delayed(1000).then(() => {
        val = seven;
    }));
  });

  it('should have waited for setter in previous it()', function() {
    expect(val).toBe(7); // <<------ This fails
  });
});

async/await are being compiled by the typescript compiler in this case, and jasminewd2 wraps each it() block in the control flow, so this test should have worked. However, the final assertion failed, with val still being 1.

Compiling down to ES6 and stripping out the jasmine/jasminewd2, the above translates to the following:

var webdriver = require('selenium-webdriver'),
    flow = webdriver.promise.controlFlow();

var val;

function runInFlow(fun, name) {
  return flow.execute(() => {
    return webdriver.promise.fulfilled(fun());
  }, name);
}

runInFlow(() => {
  val = 1;
  return new Promise((resolve) => {
    resolve(webdriver.promise.fulfilled(7));
  }).then((seven) => {
    runInFlow(() => {
      return webdriver.promise.delayed(1000).then(() => {
        val = seven;
      });
    }, 'set outer');
  });
}, 'set inner');

runInFlow(() => {
  console.log('RESULT: val = ' + val); // 1, should be 7
}, 'log');

Basically, by putting a webdriver promise inside an ES6 promise inside a webdriver promise, we somehow break the control flow. This is a problem because await compiles down to an ES6 promise, and async functions then return those promises. So if you await some webdriver promise, and then wrap the async function in the control flow, you will run into this bug (as in the first example). This means that Protractor users (or any users who wrap blocks of code in the control flow) basically cannot await an element.getText() command or any other webdriver promise or else everything will become desynchronized.

I know that as per #2969 you plan on removing ManagedPromise/the control flow entirely. But in the mean time, async functions are practically unusable, so this seemed worth bringing to your attention.

[jleyba]

Your example is working as intended. The control flow synchronizes actions within "frames", which are tied to the JavaScript event loop (as best as the control flow can). Will comment further using your code sample:

runInFlow(() => {
  // Frame 1: this is the initial frame.
  // The control flow will synchronize tasks and callbacks attached to managed
  // promises. The control flow is not able to track native promises.

  val = 1;
  return new Promise((resolve) => {
    resolve(webdriver.promise.fulfilled(7));
  }).then((seven) => {

    // This is a callback on a native promise and runs in its own turn of the JS event loop.
    // Since it is from a native promise, the control flow does not know it was chained from
    // frame 1, so the control flow creates an independent task queue. All tasks and managed
    // promise callbacks within this frame will be synchronized with each other, but not against
    // Frame 1.
    //
    // Return the promise result of this task to make every synchronize.
    runInFlow(() => {
      return webdriver.promise.delayed(1000).then(() => {
        val = seven;
      });
    }, 'set outer');
  });
}, 'set inner');

// This task is scheduled in Frame 1, so it will not execute until "set inner" completes.
runInFlow(() => {
  console.log('RESULT: val = ' + val); // 1, should be 7
}, 'log');

Same with your first example, add an await/return to link everything up:

describe('async function + control flow', function() {
  var val;
  var seven;
  it('should wait for this it() to finish', async function() {
    val = 1;
    seven = await webdriver.promise.fulfilled(7);
    await controlFlow.execute(() => {
      return webdriver.promise.delayed(1000).then(() => {
        val = seven;
    }));
  });

  it('should have waited for setter in previous it()', function() {
    expect(val).toBe(7); // <<------ This fails
  });
});

[sjelin]

I recolonize that this is unlikely to change, and if you want to close this issue I understand, but I do want to push back a bit.

I know that you're not supposed to mix managed promises with other promises, but this bug only happens when the native promise is resolved to a managed promise. For instance, this code example works fine:

runInFlow(() => {
  // Frame 1
  val = 1;
  return new Promise((resolve) => {
    resolve(7); // Here we replaced `webdriver.promise.fulfilled(7)` with `7`.
  }).then((seven) => {
    // This is a callback on a native promise, yet the control flow has no difficulty
    // seeing that it was chained from frame 1.  Why the difference?
    runInFlow(() => {
      return webdriver.promise.delayed(1000).then(() => {
        val = seven;
      });
    }, 'set outer');
  });
}, 'set inner');

runInFlow(() => {
  console.log('RESULT: val = ' + val); // 7, as expected
}, 'log');

Presumably the control flow works in this case because runInFlow wraps the returned value in a managed promise. In other words, the native promise doesn't resolve until the then() block finishes, so the managed promise won't resolve until then either. For some reason, putting a managed promise in the first block breaks this however.

Regardless, the core issue is that users want to use async/await, but experimenting with it will cause them problems that they probably won't understand or expect.

So for instance a user might have:

describe('webdriver tests', function() {
  it('test page A', function() {
    browser.get('https://mysite.com/pageA.html');
    elem = browser.findElement(webdriver.By.id('myElem'));
    elem.click();
    // more test code for page A
  });
  it('test page B', function() {
    browser.get('https://mysite.com/pageB.html');
    // test code for page B
  });
})

Then they might experiment with async/await in the following way:

describe('webdriver tests', function() {
  it('test page A', async function() {
    browser.get('https://mysite.com/pageA.html');
    elem = browser.findElement(webdriver.By.id('myElem'));
    if (await elem.getText() == 'click me') {
      elem.click();
    }
    // more test code for page A
  });
  it('test page B', function() {
    browser.get('https://mysite.com/pageB.html');
    // test code for page B
  });
})

But now the control flow is broken and their text code for page A will actually be run against page B.

Regardless of if it's working as indented, the difference is opaque to users, and the results are bizarre. We want our users to be experimenting with async/await (especially in light of #2969), but this is going to be a problem for them. And it's only going to be more of a problem as promises get more integrated with JS in subtle ways.

Might there be a way to deal with this issue specifically for native promises? Like, look for the special case of a native promise sandwiched between two managed promises and deal with it more intelligently?

[jleyba]

It's a timing issue with how the control flow tracks individual turns of the js event loop (which dictates when actions are linked). With your initial example, that first promise just needs to be asynchronously resolved - reproduces with native promises. I really want to sweep this under the rug, but given the control flow isn't going anywhere for a while, I'll see if I can track it down.

'use strict';

const assert = require('assert');
const {promise} = require('selenium-webdriver');
const flow = promise.controlFlow();

describe('timing issues', function() {
  function runInFlow(fn) {
    return flow.execute(() => {
      return promise.fulfilled(fn());
    });
  }

  function runTest(seedFn) {
    let value = '';
    return new Promise(resolve => {
      flow.once('idle', resolve);

      runInFlow(() => {
        value += 'a';

        return seedFn().then(() => {
          value += 'b';

          runInFlow(() => {
            value += 'c';
            return promise.delayed(500).then(() => {
              value += 'd';
            });
          });
        })
        .then(_ => value += 'e');
      });

      runInFlow(_ => value += 'f');

    // e before df b/c native promises won't wait for unlinked control flow result.
    }).then(() => assert.equal(value, 'abcedf'));
  }

  function test(seedFn) {
    it(seedFn + '', () => runTest(seedFn));
  }

  test(() => Promise.resolve());
  test(() => Promise.resolve(new Promise(r => r())));
  test(() => new Promise(r => r()));
  test(() => new Promise(r => r(Promise.resolve())));
  test(() => new Promise(r => r(new Promise(r => r()))));
  test(() => new Promise(r => setTimeout(() => r(), 10)));
});

$ mocha promise_test.js 


  timing issues
    ✓ () => Promise.resolve() (513ms)
    ✓ () => Promise.resolve(new Promise(r => r())) (506ms)
    ✓ () => new Promise(r => r()) (507ms)
    1) () => new Promise(r => r(Promise.resolve()))
    2) () => new Promise(r => r(new Promise(r => r())))
    3) () => new Promise(r => setTimeout(() => r(), 10))


  3 passing (3s)
  3 failing

  1) timing issues () => new Promise(r => r(Promise.resolve())):

      AssertionError: 'abcefd' == 'abcedf'
      + expected - actual

      -abcefd
      +abcedf

      at Promise.then (promise_test.js:38:26)

  2) timing issues () => new Promise(r => r(new Promise(r => r()))):

      AssertionError: 'abcefd' == 'abcedf'
      + expected - actual

      -abcefd
      +abcedf

      at Promise.then (promise_test.js:38:26)

  3) timing issues () => new Promise(r => setTimeout(() => r(), 10)):

      AssertionError: 'abcefd' == 'abcedf'
      + expected - actual

      -abcefd
      +abcedf

      at Promise.then (promise_test.js:38:26)

[sjelin]

Oh I see. So then any attempt to use async function() within the control flow will fail as long as there really is asynchronous activity.

[sjelin]

For what it's worth, we're currently playing with the idea of letting Protractor users overwrite the global Promise class with managed promises to allow async to work (angular/jasminewd#61 angular/jasminewd#62). Obviously there are numerous problems with this idea though.

[jleyba]

Well, the good news is I already have the code for disabling the control flow completed, I just haven't pushed it (hoping this week). So you could tell users if they want to use async, disable the control flow (there's no need for it if you're using async)

[sjelin]

Oh, great! Yeah, we'll be adding options for that as soon as you guys do

[jleyba]

FYI 3.0 has been pushed to npm, so you can disable the promise manager now.

[thorn0]

A simpler scenario than described here fails for me. I use Protractor, but as far as I can tell, all the promises it uses inside its browser.get are managed. So the only native promise here is the one that is created by await. What I'm trying to achieve is to temporarily change one of the timeouts that browser.get uses internally. This code seemingly works, but in fact it affects subsequent tests in an unpredictable way, which clearly means something asynchronous gets out of sync. Would be glad to get some hint on how to fix this. I use TypeScript with target: "es2015".

// all the HTTP requests can take longer than the waiting timeout (default: 11s)
const savedAllScriptsTimeout = browser.allScriptsTimeout;
browser.allScriptsTimeout = 25000;
await browser.get('web2.aspx/DB/METAGANTT/ALL');
browser.allScriptsTimeout = savedAllScriptsTimeout;

[sjelin]

This is exactly the same issue I describe in my original comment. The promise returned by await gets sent to jasminewd, which passes it onto the control flow, breaking everything. There is no fix. Some day (maybe this year but maybe not) protractor will support disabling the control flow, so you'll be able to use await for all async activity.

[jleyba]

Unfortunately, this isn't a bug, but a defect in the control flow's design.

Internally, the control flow maintains a separate task queue for each turn of the JS event loop:

'use strict';
const {promise} = require('selenium-webdriver');
const flow = promise.controlFlow();

flow.execute(() => {
  console.log('a');
  return promise.delayed(10);
});
flow.execute(() => console.log('b'));
flow.execute(() => console.log('c'));

setTimeout(() => {
  // This is a new turn of the event loop, so tasks are scheduled in
  // a separate queue
  flow.execute(() => console.log('d'));
  flow.execute(() => console.log('e'));
}, 0);
// a
// d
// e
// b
// c

The control flow does not automatically try to synchronize queues because you could end up with deadlocks:

'use strict';
const {promise} = require('selenium-webdriver');
const flow = promise.controlFlow();

var cont;
flow.execute(() => {
  console.log('a');
  return new Promise(resolve => cont = resolve);
});
flow.execute(() => console.log('b'));

setTimeout(() => {
  flow.execute(() => console.log('c'));

  // If the queue's were auto synchronized, this would never run,
  // causing a deadlock.
  flow.execute(() => cont());
}, 0);
// a
// c
// b

With native promises, each callback runs in a separate turn of the JS event loop, so it gets its own task queue that is not synchronized with the main flow. Going back to the example in my comment above: the example "works" if the first promise is resolved with a primitive, but "fails" if resolved with a managed promise. To understand why, we need to grab a snapshot of the control flow's internal state:

runInFlow(() => {
  val = 1;
  return Promise.resolve(7).then(seven => {
    runInFlow(() => {
      return webdriver.promise.delayed(1000).then(() => {
        val = seven;
      });
    }, 'set outer');

    console.log('flow state:\n' + flow);
  });
}, 'set inner');

// flow state:
// ControlFlow::11
// | TaskQueue::3
// | | (pending) Task::2<set inner>
// | | | TaskQueue::6
// | | Task::5<log>
// | (active) TaskQueue::10
// | | Task::9<set outer>
// RESULT: val = 7

// --- vs ---

runInFlow(() => {
  val = 1;
  return Promise.resolve(webdriver.promise.fulfilled(7)).then(seven => {
    runInFlow(() => {
      return webdriver.promise.delayed(1000).then(() => {
        val = seven;
      });
    }, 'set outer');

    console.log('flow state:\n' + flow);
  });
}, 'set inner');

// flow state:
// ControlFlow::18
// | TaskQueue::3
// | | (blocked) Task::2<set inner>
// | | Task::5<log>
// | TaskQueue::11
// | | (pending) Task::10<then>
// | | | TaskQueue::14
// | | Task::13<then>
// | (active) TaskQueue::17
// | | Task::16<set outer>
// RESULT: val = 1

Both cases are actually working correctly (as the control flow is designed). The first example has fewer items in the queue, and it just happens that the timers are scheduled in a way so Task::9<set outer> consistently runs before Task::5<log>. They aren't linked, it's just a byproduct of execution order (I haven't been able to write a similar scenario with just setTimeout calls, but I know it's possible).

You can use managed promises alongside native promises, you just can't mix async and sync styles. If you want to use them together, you need to treat managed promises as native: explicitly return to ensure things are properly linked:

runInFlow(() => {
  val = 1;
  return Promise.resolve(webdriver.promise.fulfilled(7)).then(seven => {
    // *** NOTE THE PROMISE RESULT IS EXPLICITLY RETURNED HERE ***
    return runInFlow(() => {
      return webdriver.promise.delayed(1000).then(() => {
        val = seven;
      });
    }, 'set outer');

    console.log('flow state:\n' + flow);
  });
}, 'set inner');
// RESULT: val = 7

As it stands, I don't see a way to fix the async/await case without introducing potential deadlocks (as outlined above).

[thorn0]

@sjelin What if make jasminewd wait for all the scheduled control flow tasks to finish before proceeding to the next test? Is it possible? This won't solve everything, but at least tests won't influence one another.

[sjelin]

@thorn0 Good idea. I made angular/jasminewd#68 to track it. Should have a PR in later today

[jleyba]

Quick thought: I could change things (or add an option to opt-in to this behavior) so if a task is pending, any new tasks will block the completion of that task.

The goal would be to end up with something like this:

flow.execute(() => {
  console.log('a');
  flow.execute(() => console.log('a.1'));
  flow.execute(() => console.log('a.2'));
});
flow.execute(() => console.log('b'));
flow.execute(() => console.log('c'));

setTimeout(() => {
  // These run in a separate q and are not blocked by the first task (a),
  // BUT they are scheduled while (a) is pending, so their completion
  // blocks the completion of (a).
  //
  // In other words, a.1 will always come before a.2 and d before e, but
  // there are no guarantees on the order of a.1 relative to d/e.
  flow.execute(() => console.log('d'));
  flow.execute(() => console.log('e'));
}, 0);

Probably still possible to introduce a deadlock with this - need to think through the implications some more.

[thorn0]

@jleyba But it will guarantee that d and e will come before b and c, right?

[sjelin]

I think this could introduce deadlock:

var task = flow.execute(() => {});
setTimeout(() => {
  flow.execute(() => {
    return new ManagedPromise((done) => {
      task.then(done);
    });
  });
}, 0);

[mmc41]

Any updates on this issue - I have trouble getting await to work in my angular2 e2e tests + typescript gets confused because there are two types of Promises around.

[mmc41]

@jleyba About your comment "FYI 3.0 has been pushed to npm, so you can disable the promise manager now.". Can you elaborate/explain how and if it solves the problems raised here?

[jleyba]

The promise manager has been removed from the code base (5650b96).

This will be included in the 4.0 release (which won't go out until Node releases their next LTS later this month).

[4iAmAve]

@jleyba

is there any specific date already for the release 4.0 since this is causing our tests to fail.

[arjunUnniumbath]

My code base is in Limbo state and I'm unable to debug. Any update on this guys? @jleyba Thank you for your time.

[jleyba]

How are you in limbo? selenium-webdriver@4.0.0-alpha.1 was published 4 months ago.

http://seleniumhq.github.io/selenium/docs/api/javascript/page/Changes.html