Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Async hooks run in parallel to the collection operation #301

Open
pmogollons opened this issue Sep 23, 2023 · 3 comments
Open

Async hooks run in parallel to the collection operation #301

pmogollons opened this issue Sep 23, 2023 · 3 comments
Labels
bug good first issue This is an easy issue that beginners can tackle to get to know the code base.

Comments

@pmogollons
Copy link

pmogollons commented Sep 23, 2023

Describe the bug
Async hooks dont wait for the hook to return to perform the Collection op, for example.

To Reproduce

Tickets.before.insert(async function ticketsBeforeInsert(userId, doc) {
  doc.idNumber = await incrementCounter(`PQR::${doc.orgId}`);
});

console.log(doc.idNumber) // This will be undefined

Expected behavior
Async hooks should await for the response before calling the collection op.

Additional context
Happens in all of the package versions.

Workaround
In a Meteor version before 3.0 you can just use Promise.await

Tickets.before.insert(function ticketsBeforeInsert(userId, doc) {
  doc.idNumber = Promise.await(incrementCounter(`PQR::${doc.orgId}`));
});

Possible way to fix it
When we call the aspect function callbacks (hooks) we should await them, this will require to use async on CollectionHooks.defineAdvice and to use await Promise.all on each aspect.

For example, this are the changes done to the insert advice:

      await Promise.all(aspects.before.map(async (o) => {
        const r = await o.aspect.call({ transform: getTransform(doc), ...ctx }, userId, doc)
        if (r === false) abort = true
      }))

The issue with this is that this makes the collection methods to be async too and test will fail.

Any ideas on how to approac this issue will be appreciated.

Related issues
#71

@github-actions
Copy link

Thank you for submitting this issue!

We, the Members of Meteor Community Packages take every issue seriously.
Our goal is to provide long-term lifecycles for packages and keep up
with the newest changes in Meteor and the overall NodeJs/JavaScript ecosystem.

However, we contribute to these packages mostly in our free time.
Therefore, we can't guarantee you issues to be solved within certain time.

If you think this issue is trivial to solve, don't hesitate to submit
a pull request, too! We will accompany you in the process with reviews and hints
on how to get development set up.

Please also consider sponsoring the maintainers of the package.
If you don't know who is currently maintaining this package, just leave a comment
and we'll let you know.

@StorytellerCZ StorytellerCZ added bug good first issue This is an easy issue that beginners can tackle to get to know the code base. labels Sep 23, 2023
@pmogollons
Copy link
Author

@StorytellerCZ any ideas where to look? I fiddled around with it yesterday with no luck. I think one of the possibilities to fix this is to only support this use case in Async methods, so I added a new insertAsync advice, but it never gets called.

@vincentcarpentier
Copy link

Any news about this issue ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug good first issue This is an easy issue that beginners can tackle to get to know the code base.
Projects
Status: Investigation 🔭
Development

No branches or pull requests

3 participants