Proposal to fix issues with async method stubs

core
1

I’ve been trying to figure out how to support async method stubs in zodern:relay without the issues the current implementation in Meteor has. I think this is the most promising solution I’ve found.

Method Stubs

Method stubs is another name for Methods implemented on the client (by calling Meteor.methods on the client, importing validated methods on the client, etc.). These simulate what the server will do, allowing the user to immediately see the result. After the stub finishes, Meteor runs the method on the server, and updates the client state if the server result is different from what the stub simulated.

Many methods are not implemented on the client (some apps don’t have any stubs).

Before Meteor 2.8, all stubs were sync (they couldn’t use promises, async/await, or other async api’s).

Problem

Meteor 2.8 added support for async method stubs. The current implementation has a few limitations that will hopefully be fixed. There’s been a number of discussions about it over the last year, but there isn’t a good solution to it.

When running a stub, Meteor creates a simulation to track what the stub does and simulate what the server will do. This simulation is global - any code that runs is inside the simulation. With sync stubs this works fine - no other code is able to run in the simulation. However, with async stubs it is very easy for outside code to run during a simulation.

During a simulation, some of Meteor’s api’s behave differently, which can cause unexpected results when code outside of the method stub uses them:

  • Meteor.call, Meteor.apply, Meteor.applyAsync only run the stub when called in a simulation (they expect the server implementation of the first method will also call them)
  • Due to a bug, Meteor.callAsync errors if called during a simulation
  • Modifications to a collection (Collection.insert, Collection.update, etc.) in a simulation are tracked. When the server result from the method is received, the modifications are reverted or updated to match what the server did. For apps that modify collections outside of stubs (using the allow-deny package) this can cause changes to be lost if they happen to be made while a simulation is running.
  • Some Meteor apis do not allow themselves to be used while a simulation is running.

Currently, the recommendation is to use async/await or .then callbacks to ensure the app only calls one method at a time and wait until it receives the server result before calling another method. For some apps this is difficult to check for - there are many ways multiple methods could be called at the same time in different files or in different functions that can require a good understanding of the browser and js libraries used to identify.

Solution

I created a package that modifies Meteor to implement this proposal. Add it with:

meteor add zodern:fix-async-stubs

Then you can call methods whenever and however you want without worrying about the problems mentioned above.

For it to completely remove those problems, async method stubs can not use some web api’s:

  • fetch/XMLHttpRequest
  • setTimeout or setInterval
  • indexedDB
  • web workers
  • any other async web api that schedules and waits for macrotasks

If an async method stub does use one of these, a warning is logged to the console with a link to the docs.

How it works

Browsers have two queues - a macrotask queue and a microtask queue (mostly for promises) that it runs during the event loop. It runs one macrotask at a time. When the task finishes, it then runs all microtasks until the microtask queue is empty. While a macrotask and the microtasks scheduled by it is running, unrelated code does not run.

This proposal is to schedule a separate macrotask to run each async stub in. As long as the method stub only schedules and waits for microtasks (no macrotasks), it will finish running before any other app or package code can run.

The api’s that only support sync stubs (Meteor.call and Meteor.apply) run the stub immediately. This is still possible (my package does), as long as the methods are still run on the server in the original call order. This means that the stubs could run in a different order than the methods run on the server increasing the chance of the stubs producing different results than the server. Depending on how large of a breaking change it is, it might be possible to only run a stub out of order when Meteor.apply is used with the returnStubValue option.

Risks and problems

  1. 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.
  2. This requires browsers to be adequately spec compliant for the event loop and how various api’s interact with it. For example, this would not work in old versions of Firefox (I think it was fixed in Firefox 58).
  3. Can we clearly explain how to write async method stubs, and show a warning message that new developers can understand?
  4. It does restrict what api’s method stubs can use. Are these restrictions reasonable or easy to work around if necessary?
    1. Personally, I don’t think method stubs should be using these api’s anyway. These make a simulation take several macrotasks. This allows unrelated macrotasks to run unrelated app/package code for event listeners, setTimeout callbacks, or other code while the simulation is running. Even if you use async/.then to run one method at a time, that isn’t enough when method stubs use these api’s.
10 Likes
2

To be frank … I am completely lost :slight_smile:
Could you please exemplify when/how/ in what conditions Meteor creates a simulation and opposed to it, when it doesn’t.

While I understand what a method stub is in general, I don’t think I understand the actual problem and consequently I do not understand if I am being affected by this or not.

Ok … I checked the package you propose. So does these async stubs issue only affect Collections operations initiated on the client?

3

When calling a method on the client, the method either has a client implementation (also called a stub) or it doesn’t.

When a method does not have a client implementation, the call is sent to the server and the method is run there.

When a method does have a client implementation/stub, Meteor creates a simulation and runs the stub. After the stub finishes, it then runs the method on the server.

Meteor always creates a simulation when running the stub. If your apps don’t use stubs and don’t use packages that have stubs, then you do not have this issue.

The 2.8 migration guide has a number of examples of when the problem can happen. While a method stub is running, any code outside of the method stub can not use:

  • Meteor.call, Meteor.apply, Meteor.callAsync, Meteor.applyAsync, Collection.insert, Collection.update, Collection.remove, etc.

This proposal is a way to allow safely using those api’s without worrying if a method stub is currently running.

3 Likes
4

I updated the initial post. Hopefully it is a little more clear.

5

Ok, a method can be created in a file in a server folder or in a “common” folder. If a method is in a server folder, is it automatically not having a stub?

To call a method on the client is to have a method defined in a “common” folder and if the method defined is in a server folder then the method call is "just like remote procedure calls " and there are no worries with respect to this thread?

"
Calling methods on the client defines stub functions associated with server methods of the same name. You don’t have to define a stub for your method if you don’t want to. In that case, method calls are just like remote procedure calls in other systems, and you’ll have to wait for the results from the server.
"

I am really sorry if I sound a bit … slow but the Meteor documentation must have initially been written by Dostoevsky and then updated by Dickens. There is just so much phrasing for everything.

6

There does seem to be some gaps in the docs related to this.

There is a stub when a method is created on the client with the same name as the method on the server.

If you import the common file on both the client and server, then on the client it will create a method with the same name as on the server, and there is a stub.

For the server file example, by itself there would be no stub. However, you could have a separate file on the client that creates a method with the same name, which would create a stub.

3 Likes
7

Clear explanation and solution. Thanks for creating this.

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

From the looks of it, even the current stub explanation without the async limitation is already difficult to explain. When I started using Meteor, I mostly created stubs even without needing it just because the methods are imported in the client.

Good point on why these methods shouldn’t be on stubs anyway.

From my personal experience, the default way of using methods in the documentation should be without stubs (including why you shouldn’t be including method definitions in client if you don’t need them). Then followed by an advanced method of using stubs with explanation what are the benefits e.g. optimistic UI. The limitations can now be included in the advanced section and not with the default way of explaining/using methods

Overall, this removes huge complexity in using stubs even with the limitations on web apis.

4 Likes
8

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

That’s the way to go, I’d say. Yes, it’s limiting, but we could call it “opinionated” instead. In other words, there are limitations, but you get a nice optimistic UI experience in return.

As for the code, I’d say it looks correct. It’s also clear that it could be simplified if merged into the Core. What I think we need now is to maybe roll it out as a part of the next release so people could try it out in the wild? As I wrote on Slack:

I checked our codebase now and even async validation from Ajv or Zod would be an issue there. Another problematic thing is async toolbelts (e.g., p-* family from Sindre Sorhus) as they can rely on setTimeout sometimes (so is data-loader).

However, it should not be a problem in general but “only” a migration burden.

Great job @zodern!

1 Like
9

Great work zodern!

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.

With async stubs on the client and promised-based packages like dexie, my hope was that it would make it easier to have offline persistence with minimongo out of the box with maybe some minimal configuration.

@banjerluke, you previously shared some code for offline persistence using dexie, curious if you have any thoughts here?

10

@jam I’m not sure how much insight I can offer because I stopped using client-side method stubs in my app years ago. When I have offline mode enabled (it’s currently on my default on mobile and desktop, opt-in on the web) then database operations happen client-side only (with IndexedDB via Dexie) and those operations get added to a queue to sync with the server. I am having a hard time imagining a useful combination of client-side stubs and IndexedDB-backed databases, mostly because (in my experience) offline-first data storage and manipulation needs to be a bit more bespoke and thought-out. Just my hot take on this topic.

2 Likes
11

Great work @zodern.

The insight of using microtasks is genius!

I wouldn’t be concerned about education at this point. We need to find a suitable solution that works well.

Then, our job (community and Meteor Software) is to create/update materials to explain how they work and when you should use them.

Simulations will work much better using this microtask insight than a global control. At least devs will be able to understand from the first principles of the platform (Javascript Engines/Browsers/Node.js) why we have such limitations.

Node.js/Javascript devs need to learn about Event Loop and the internals of these tools.

Follow below a snippet of our internal education material for Quave Developers, where we list a bunch of videos about this topic:

  • Non-blocking I/O and how Node uses it, in friendly terms: blocking vs async IO, CPU vs IO - Studying with Alex on YouTube
  • Event Loop - Jake Archibald on YouTube
  • JavaScript VM internals, EventLoop, Async and ScopeChains - Arindam Paul on YouTube
  • What the heck is the event loop anyway? - Philip Roberts on YouTube
  • Everything You Need to Know About Node.js Event Loop - Bert Belder on YouTube
  • The Node.js Event Loop: Not So Single Threaded - Bryan Hughes on YouTube
  • Node’s Event Loop From the Inside Out - Sam Roberts on YouTube
  • Broken Promises - James M Snell on YouTube
  • Why does node.js scale? Libuv & epoll & fcntl - Gabriel Zimmermann YouTube

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?

I suggest you create a PR (against release-3.0?), and then we continue the review/code from there. What do you think?

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.

2 Likes
12

Hi Zodern!

I think this is amazing, and if this works, it’ll be a great way to add some faster UI reaction times back into the UI in strategic places!

I think “macrotask” style callbacks wouldn’t have made a lot of sense before, at least not when one would be waiting for the result / effects of a stub somehow… that’d have been counter productive before as well I think.

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

I think if it’s possible to use the plugin to the same effect then it’s fine to only integrate in 3.0 proper. But I think there should be a way for people to incrementally switch their code to Async without having to fully switch to Meteor 3 first.

The plugin is a really nice way of doing this kind of augmentations at this point I think so people can add stuff & experiment without having to wait for new releases etc.

Great stuff @zodern , we’ll add this to our codebase this week and check it out.

1 Like
13

@zodern In our codebase I fixed this issue by monkey-patching applyAsync and execute all async methods on the client via a queue. This way there is no need to worry about micro vs macro tasks. Here is how it looks:

const queue = new PromiseQueue();

const originalApplyAsync = Meteor.applyAsync;
const originalCallAsync = Meteor.callAsync;

Meteor.applyAsync = (...args) => {
  // NOTE: make sure only isomorphic methods are globally executed in sequence.
  if (Meteor.connection._methodHandlers[args[0]]) {
    return queue.enqueue(() => originalApplyAsync(...args));
  }

  return originalApplyAsync(...args);
};

Meteor.callAsync = (...args) => {
  // NOTE: make sure only isomorphic methods are globally executed in sequence.
  if (Meteor.connection._methodHandlers[args[0]]) {
    return queue.enqueue(() => originalCallAsync(...args));
  }

  return originalCallAsync(...args);
};

And the PromiseQueue class:

export class PromiseQueue {
  queue: Array<number>;

  lastId: number;

  currentRunningStack: number[];

  constructor() {
    this.queue = [];
    this.lastId = 0;
    this.currentRunningStack = [];
  }

  enqueue<T extends Promise<any>>(factory: () => T): T {
    this.lastId += 1;

    const id = this.lastId;
    this.queue.push(id);

    // NOTE: a promise should only track dependencies created before its creation, as
    // the stack is mutable and can grow in time.
    const savedStack = [...this.currentRunningStack];

    return new Promise((resolve, reject) => {
      const interval = setInterval(() => {
        const deps = this.queue.filter(el => el < id && !savedStack.includes(el));

        if (deps.length > 0) {
          return;
        }

        clearInterval(interval);

        this.currentRunningStack.push(id);

        factory()
          .then(data => {
            this.currentRunningStack.pop();
            resolve(data);
            remove(this.queue, el => el === id);
          })
          .catch(error => {
            this.currentRunningStack.pop();
            reject(error);
            remove(this.queue, el => el === id);
          });
      }, 10);
    }) as T;
  }
}
2 Likes
14

That was my first idea as well. Do you use nested method calls in stubs, though? Something like this:

Meteor.methods({
  async foo() {
    console.log(1);
    await Meteor.call('bar');
    console.log(5);
  },
  async bar() {
    console.log(2);
    await Meteor.call('qux');
    console.log(4);
  },
  async qux() {
    console.log(3);
  },
});

Meteor.call('foo');

By quickly looking at your code, it may hang here since foo is not done until bar is, and bar won’t start since it’s waiting for foo.

15

We do use nested calls, and the code above handles it correctly. That’s tracked via “currentRunningStack”, “savedStack” variables.

1 Like
16

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

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.

17

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 :smiley:

1 Like
18

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.

19

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.

5 Likes
20

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

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

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.

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.

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.

4 Likes