Proposal to fix issues with async method stubs

[DanielDornhardt]

That’s really cool @alisnic where were you three weeks ago when we started ripping out all our stubs?

Just kidding, if that works that’s really great and we’ll definitely check that out. It also could be turned into a package for people to play with quite easily I think…

And/or of course be included in the core for Meteor 3 possibly.

[alisnic]

I was thinking about making a PR to Meteor, but the whole approach of setInterval seems a bit hacky to me and I thought Meteor core team will figure out something better in meantime

[DanielDornhardt]

I can’t speak to that, but if it works, it works… looking forward to some other people looking over this & trying it out in their projects, ideally.

Should be easy to test by using copy&paste in the code too.

[denyhs]

That’s great! @grubba will create two PRs, one implementing Zodern’s solution and the other with @alisnic’s solution. The core tests will run in both of them, and we can analyze to see which will work best, which will cover more edge cases, etc.

[zodern]

@rjdavid

@zodern

This proposal is reliant on some assumptions about how browsers and the event loop work. It is possible my understanding of these is incorrect, or future changes to the specifications could break these assumptions.

1. If this happens, it most likely would be specific api’s that break it. We could probably wrap those api’s to ensure they don’t run app/package code while an async stub is running.

Are you talking about web/browser apis or Meteor apis?

Web apis. As @radekmie mentioned it is unlikely to be a problem.

@radekmie

I wouldn’t be too bothered by that – it’s part of the HTML standard and Node.js promises similar behavior. (It doesn’t mean it won’t change at some point, though.)

My concern is about the browser creating unrelated microtasks while a stub is running. Looking at the spec again it seems like it would mostly prevents this. The exceptions would be api’s like MutationObserver that create a microtask to run a callback in response to something the stub did (though this specific case shouldn’t be a problem since stubs shouldn’t modify the DOM).

@jam

I think the only potential downside that I can see is preventing using async stubs with indexedDB (or their related wrappers e.g. dexie) for offline persistence. If I have this wrong, please let me know.

I’ve looked into this several times in the past, and as @banjerluke described it doesn’t seem the current design for method stubs is a good solution. You might be able to after redesigning how method stubs work, but then you could also design them to work well with indexedDB.

I don’t have much experience with indexedDB, and the spec isn’t very clear on how it interacts with the event loop, but it seems like reading might work, and writing should work. The main problem is that the stub can’t wait to see if the transaction succeeded or not.

@filipenevola

Getting back to your code, I’ve reviewed it, and it looks good, but you still have some parts that you would like to improve, right?

The wait option was broken with Meteor.apply, but I published a new version today to fix it. Besides that, I think it is done.

@filipenevola

Do you want to support Meteor 2.x as well? I don’t think the effort is worth it since most users are ok with it.

If it is fixed in Meteor 3, I think it should also be fixed in Meteor 2 so apps can migrate to async stubs without the current problems. My projects still use sync stubs and I know a few other developers are doing the same, so fixing it would allow those projects to use async stubs before updating to Meteor 3.

[zodern]

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.

[grubba]

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

[rjdavid]

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

[alisnic]

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.

[rjdavid]

@rjdavid

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

Add zodern solution for Async stubs

[waldgeist]

@banjerluke

When I have offline mode enabled (it’s currently on my default on mobile and desktop, opt-in on the web)

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

[waldgeist]

That’s great stuff, thanks for sharing!

[filipenevola]

@waldgeist

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

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).

[banjerluke]

@waldgeist

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

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.

[waldgeist]

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

[grubba]

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

Add alisnic solution for Async stubs

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

[zodern]

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.

[zodern]

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.

[DanielDornhardt]

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.

[harry97]

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?