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

Bull - Scoped Jobs #5

Closed
maphe opened this issue Aug 18, 2020 · 10 comments
Closed

Bull - Scoped Jobs #5

maphe opened this issue Aug 18, 2020 · 10 comments
Labels
enhancement New feature or request

Comments

@maphe
Copy link

maphe commented Aug 18, 2020

Is your feature request related to a problem? Please describe.
We're experiencing issues with Bull where the Entity Manager is shared across all running jobs. That leads to errors memory leaks.

Describe the solution you'd like
There is this open issue on the nestjs/bull side which might be a dependency here, but I ideally would like pretty much the same mechanism as RequestContext middleware but applied to Bull Jobs (which obviously are not considered Requests by nest)

Describe alternatives you've considered
I don't have any ideas for alternative but I'm happy to hear some if you do.

@maphe maphe added the enhancement New feature or request label Aug 18, 2020
@B4nan
Copy link
Member

B4nan commented Aug 19, 2020

You should never use the global EM, always create a fork. If it's not possible to leverage RequestScope, then you need to create the fork manually I guess.

Btw it should be possible to use the RequestScope helper manually too, in latest version you also have RequestContext.createAsync:

    await RequestContext.createAsync(orm.em, async () => {
      // here it should be safe to use EM or repositories from DI
	  // so you could just wrap your handlers in this?
    });

@maphe
Copy link
Author

maphe commented Aug 19, 2020

Thanks for the tip, that sounds like something that would make a good method decorators. Bull workers are defined via decorators so I guess I could have some kind of @OrmIsolated decorator or something of this kind, that would just wrap the function call into a temporary context.

Only thing is I'm not sure how I'd access the orm.em from there 🤔 is there some kind of static helper so I can get the global em without injecting it?

@B4nan
Copy link
Member

B4nan commented Aug 19, 2020

Nope, no global state out of box, but you could probably get around that yourself, defining some global export where you store the ORM instance once it's ready.

@maphe
Copy link
Author

maphe commented Aug 19, 2020

Ok thanks. I'll look into it. Might come back here with some more questions tho. But really appreciate the help.

@maphe
Copy link
Author

maphe commented Aug 20, 2020

Just sharing here what we ended up doing:

export function IsolateOrm() {
  return function(
    target: any,
    propertyKey: string,
    descriptor: PropertyDescriptor,
  ) {
    const originalMethod = descriptor.value;
    descriptor.value = async function() {
      const context: any = this;
      const args         = arguments;

      if (!(context.orm instanceof MikroORM)) {
        throw new Error('@IsolateOrm decorator can only be applied to methods of classes that carry `orm: MikroORM`');
      }

      await RequestContext.create(context.orm.em, async () => {
        await originalMethod.apply(context, args);
      });
    };
    return descriptor;
  };
}

So the only constraint that's added is to inject MikroORM to our job consumers before applying the decorator to the worker methods.

Might be worth having something similar built in here, I don't know.

@B4nan
Copy link
Member

B4nan commented Aug 20, 2020

Feel free to send a PR. I'd use a bit different naming, @UseRequestContext() maybe?

Also you might want to use RequestContext.createAsync that will actually await the callback, as RequestContext.create will not return a promise.

@maphe
Copy link
Author

maphe commented Aug 20, 2020

Cool, I'll do that eventually. Thanks

Like your better naming as well 👍

@maphe
Copy link
Author

maphe commented Aug 20, 2020

@B4nan the createAsync version is only available in v4, any idea when it will get released?

@B4nan
Copy link
Member

B4nan commented Aug 20, 2020

Latest rc is stable, i am already using that in production. Will ship the final later this month.

@maphe
Copy link
Author

maphe commented Aug 20, 2020

Cool, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants