Proposal to fix issues with async method stubs

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?

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

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

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.

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

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

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

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?

@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

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

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

@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

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.

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

1 Like

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.

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

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.

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

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

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