Proposal to fix issues with async method stubs

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.

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

That’s great stuff, thanks for sharing!

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

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

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

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

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

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.

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.

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?

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:

@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

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

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

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

Meteor.defer() has a different use case with async.

Meteor.defer() is for code executions that you don’t have to wait for to complete, so like a promise that you don’t wait to resolve.

async code is something you have to await, so a promise you wait to resolve

Fantastic point. Never thought of these functions if you have not enumerated them.

Hi Guys & @zodern , @grubba :slight_smile:

We’re using @zodern 's package, zodern:fix-async-stubs@1.0.2 in our app now, and we’re gonna do final testing soon, but the good news is that

a) it doesn’t seem to impact anything method-wise negatively which isn’t related to async stubs in the client, so we could just add it & our app keeps on working, tests are working too
b) we’ve selectively re-added the client side stubs to slightly longer running methods which would profit from a lil’ client side simulation speed boost, and so far we had no issues!

We’re still doing more QA testing, but I expect we’ll put it in production next week, including the new package…

We started with @zodern 's package as he got here first :slight_smile:

4 Likes