Skip to content
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

Microtask update introduced event dropping #332

Closed
rwjblue opened this issue Feb 27, 2018 · 5 comments · Fixed by #335
Closed

Microtask update introduced event dropping #332

rwjblue opened this issue Feb 27, 2018 · 5 comments · Fixed by #335
Labels

Comments

@rwjblue
Copy link
Collaborator

rwjblue commented Feb 27, 2018

Copied from the excellent writeup @ef4 did in emberjs/ember.js#16296:


Hello fans of timing bugs, today I have a fun one for you.

The new microtask-based autorun architecture can drop scheduled events. To see it in action, you can run the following against 3.1 beta:

import Controller from '@ember/controller';
import { schedule } from '@ember/runloop';

export default Controller.extend({
  actions: {
    testIt() {
      // Here I am deliberately escaping the run loop to use autoruns instead.
      Promise.resolve().then(() => {
        // This schedules an autorun
        schedule('render', () => {
          console.log(1);
          // Here we schedule a microtask. It will be ahead of the runloop's own next flush.
          Promise.resolve().then(() => {
            console.log(2);
            return Promise.resolve().then(() => {
              // This runs fine
              console.log(3);
              // But this scheduled event never fires.
              schedule('actions', () => console.log('THIS NEVER RUNS!!!!') )
            });
          })
          // when we return out of here, the runloop goes back to the 
          // first queue to start flushing again. But our promise stack is
          // interleaving with the runloop's one-microtask-per-queue.
        });
      });
    }
  }
});

If you fire the testIt event, the console logs

1
2
3

The number of layers of promises required to experience the bug depends on which queue you're trying to schedule. The runloop spends one microtask per queue, so our stack of promise resolutions is interleaved with each queue's flush. In the above example, we schedule on the actions queue immediately after that queue has just flushed, and the runloop never notices.

You can make it drop events on any other queue just by varying the number of promises you resolve in between the time you first trigger an autorun and the time you schedule your doomed event.

I published a working example of the bug here: https://github.com/ef4/bug-repro/tree/bad-timing

@rwjblue
Copy link
Collaborator Author

rwjblue commented Mar 9, 2018

Closed by #335

@rwjblue rwjblue closed this as completed Mar 9, 2018
@ef4 ef4 mentioned this issue Mar 17, 2018
@rwjblue rwjblue reopened this Mar 17, 2018
ef4 added a commit that referenced this issue Mar 17, 2018
Now that flushing can be "paused", the earlier logic for re-checking earlier queues is insufficient. This change switches to a more robust strategy -- instead of going back whenever a queue is found with work in it, we go back whenever a new task gets scheduled.
@Dhaulagiri
Copy link

I'm seeing a small amount of test failures in the Ember 3.1 betas where async test helpers are not resolving in the proper order, would this issue be related?

@rwjblue
Copy link
Collaborator Author

rwjblue commented Mar 22, 2018

The BB update to use microtasks was only included in 3.1.0-beta.1 through beta.3, it is not included in beta.4 or higher (and will not be included in 3.1.0 final). The update was specifically reverted due to this bug report (which we think we have fixed btw).

@Dhaulagiri
Copy link

@rwjblue excellent, thanks for confirming. I'll open an issue against Ember then, although I'm at a bit of a loss as to how best to get a useful reproduction other than the tests used to work and now they don't 🌵

@rwjblue
Copy link
Collaborator Author

rwjblue commented May 31, 2018

This was fixed by #336, and the version of backburner using microtasks is included by Ember since 3.2.0-beta.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants