Proposal to fix issues with async method stubs

core
21

Thanks for sharing @alisnic. The currentRunningStack/savedStack with running each stub in a separate macrotask prevents the common causes of the issues better than I had expected.

I might be misunderstanding the code, but it seems macrotasks are still important. If a stub runs across multiple macrotasks, the issues still exist for apps that use the allow-deny package or Meteor’s api’s that don’t allow themselves to be used in stubs.

This can also affect outside code calling a method while an async stub is running. When calling a method that doesn’t have a stub, or when using Meteor.call or Meteor.apply, it immediately calls the method which causes them to be part of the simulation. For methods without a stub and maybe when using Meteor.call it can queue it to fix the problem.

Meteor.apply is more difficult since it can’t be queued when using the returnStubValue or throwStubExceptions options, and at the same time it can’t run a stub when another stub is running. That is one of the main problems with my previous idea of fixing this by tracking the async context.

There seems to be one case where currentRunningStack/savedStack doesn’t prevent an outside Meteor.callAsync call from running in an async simulation: a stub creates at least two macrotasks and takes at least 10ms to run. If outside code calls a method (call 2) while an async stub is running (call 1), call 2 will include call 1 in its stack. After the 10ms delay from the interval, if call 1 is still running, then call 2 will be part of the simulation and not run on the server.

I’m not sure how common those two conditions would be. Though calling a method from a stub can create the conditions - the nested method call adds at least a 10ms delay and creates at least one macrotask.

1 Like
22

Giving this discussion, I have created 2 PRs so that we can check this code.

Note that it is not “correctly” implemented. I just pasted the code in client_convenience.js so that the Meteor picks up the patch.

When we decide to go with a solution, I will make this implementation in the correct place.

Zodern solution PR

Alisnic solution PR

23

Anybody who has a good sample code to test all the solutions against?

24

I think the best way to test is in your product. If you have end to end tests, just add the change and see how everything is running.

25
26

May I ask how you actually implemented this “offline” mode?

27

That’s great stuff, thanks for sharing!

28

We (quave) have implemented this in the past, and we may be able to create a package with this code.

Maybe you should open an issue somewhere to discuss details. Could be in Meteor itself or Quave OSS.

Our solution was working well with offline Minimongo and in an app running in production at a large scale. The only downside was the lack of indexes in Minimongo so big collections (more than 1,000 documents) were a problem in some cases (doing full-scan many times if you don’t be careful with react renders, for example).

1 Like
29

I wrote quite a bit about how I implemented offline mode, plus some source code, in this thread. Scroll down from there as I wrote multiple posts.

2 Likes
30

Thanks, that’s awesome. Will check it out!

31

FYI, @alisnic did not break any of our tests!

I will try making a PR with his solution being correctly implemented in our code.

Next I will look finding a solution to this issue here: [Meteor 3] Meteor.callAsync doesn't provide result from server · Issue #12888 · meteor/meteor · GitHub

32

When implementing it in core, you’ll want to fix the issues I mentioned in Proposal to fix issues with async method stubs - #21 by zodern.

The original problem is from outside code running during a simulation. My proposal was:

  1. Each async method stub should run in a separate macrotask. This removes the most common ways outside code can run during a simulation (including all of the issues mentioned in the 2.8 migration guide).
  2. Method stubs should not wait for other macrotasks. This should remove all of the remaining ways it can happen.

@alisnic’s solution does the first one, but currently does not do the second. You can use this code to test:

if (Meteor.isServer) {
  Meteor.methods({
    'stub-only'() {
      console.log('stub only called??');
    },
    'server-only'() {
      console.log('server-only called');
    },
    'method'() {
      console.log('method called');
    }
  });
} else {
  Meteor.methods({
    'stub-only'() {},
    async 'method' () {
      await Meteor.applyAsync('stub-only', [], {})
      await Meteor.applyAsync('stub-only', [], {})
    }
  });

  Meteor.callAsync('method');

  // with the current implementation, this will run inside the simulation
  setTimeout(() => {
    Meteor.applyAsync('server-only', [], {});
  }, 15);
}

The server should log server-only called, but since the timeout callback runs inside the simulation it does not.

I tested this code in a blank Meteor app. Interestingly, I noticed that around 10% of the time I loaded the page Meteor would fail to setup the ddp heartbeat. Sometimes the client would receive a websocket message while the simulation was running and try to start the heartbeat, which uses a Meteor api that behaves differently in a simulation.

Implementing the second part of the proposal would fix these issues.

5 Likes
33

I published zodern:fix-async-stubs@1.0.2. Meteor’s tests showed that the call order of methods and subscriptions needs to be maintained, so I added queueing for subscriptions.

How the package fixes [Unable to use Meteor.callAsync from async method stub · Issue #12889 · meteor/meteor · GitHub] also broke Meteor.isAsyncCall. I’ll fix that in a later version since at the moment I don’t understand the purpose of the api or how it is expected to behave with the queue.

34

Sorry @zodern @radekmie , stupid questions:

These microtask shenanigans you’re doing for the async stubs, couldn’t we actually use the same trick for the Meteor Tracker?

To keep the Tracker.currentComputation around if it’s all in the same microtask etc pp?

Because I think before Macrotasks wouldn’t have worked with Tracker computations either, right?

Why doesn’t this just work out of the box and could we somehow fix / use this somehow to restore Tracker to some of its previous glory?

Edit:

If people insist to use Macrotasks in their client code (oh my!), we could hack these & queue those until no Tracker.currentComputation exists, if that’d help.

35

There’s no glory in using Tracker. Get on with it, boomer! Haven’t you heard about the latest thing React/Vue/Svelte can do?

36

Why, thank you for the valuable contribution @harry97 ! I would have expected no less! :smiley:

Also, nice to see you up this early for a change! :hatching_chick: Early bird catches the worm and all that! :worm:

:stuck_out_tongue:

38

@zodern In my humble opinion calling a method via a setTimeout is a very marginal use-case, which is not worth solving given the complexity of the solution

39

How does Meteor.defer() fits into this discussion? We use a lot of methods inside Meteor.defer()

40

setTimeout was used as a reliable and simple way to demonstrate that outside code can run during a simulation. The specific api isn’t important. This could also happen with event listeners, timers, indexedDB transactions, the many web api’s that loads a resource (such as fetch), and other api’s. In my comment I described how this was sometimes happening by the event listener for websocket messages running inside a simulation.

I don’t think setTimeout is an unrealistic scenario. Many apps use timers for debouncing/throttling, autosaving, or other cases to schedule calling a method.

I don’t know how often this would happen in production. It is heavily reliant on the timing of events which is in many cases unpredictable, and only happens when method stubs run over multiple macrotasks. But it can happen and lead to very difficult to reproduce and fix bugs. For example, if you received a bug report from a user that the autosave didn’t save some data they entered, would you be able to reproduce or figure out that it was caused by the method being called while a simulation was running, and if you did how would you fix it?

If we didn’t have any other option, this might be an acceptable trade off along with some changes to make it less likely and new api’s for apps to handle it. Since we do seem to have a way to avoid this problem, I think we should.

I’m not sure, but I don’t think it would work.

Sync computations need to run immediately. Any queueing for these would break too much code (including zodern:melte). The problem is there currently isn’t a way to detect async computations before they run.

Since tracker computations are used frequently in rendering, running each one in a separate macrotask would cause more flashing and renders with out of sync data. This is because the browser is allowed to render the screen between macrotasks. By running all new computations immediately and all invalidated computations in the same macrotask tracker generally avoids these issues, and at least for Blaze it should work better with how it schedules re-renders.

edit: I realized I misunderstood your message. Meteor.defer immediately schedules a macrotask to run the callback. The callack could run inside a simulation if the simulation runs across multiple macrotasks, but the calling code could also have run inside a simulation (though maybe less likely than the callback).

Original reply:

If I understand correctly, you are using Meteor.defer outside of a method stub. In that case my proposal wouldn’t affect it.

The common use for Meteor.defer is to use inside a method to run some code without blocking the method. My proposal doesn’t affect this.

If the method stubs are waiting for Meteor.defer to run, that wouldn’t work with my proposal since Meteor.defer creates a macrotask to run the callback. If apps use Meteor.defer that way, the implementation could be changed so Meteor.defer schedules running the callback differently when inside a simulation.

3 Likes
41

Is there still a good use case for Meteor.defer in 3.0? I imagine all methods will become async.