taskforcesh / bullmq

BullMQ - Message Queue and Batch processing for NodeJS and Python based on Redis
https://bullmq.io
MIT License
5.95k stars 386 forks source link

How to set a memory limit for a sandboxed processor? #1555

Open ppedziwiatr opened 1 year ago

ppedziwiatr commented 1 year ago

I assume that Sandboxed Processors work underneath as a separate node.js Workers.

How can I pass the memory configuration to the Sandboxed processor?

E.g. the resourceLimits. maxOldGenerationSizeMb from the original Worker api https://nodejs.org/api/worker_threads.html#worker_threads_new_worker_filename_options

manast commented 1 year ago

Currently they are implemented as spawn processes, so there is no such thing as memory configuration AFAIK.

ppedziwiatr commented 1 year ago

Thanks for the answer. What is the advantage of using a spawned process (in comparison to worker threads)?

Also - when running a node.js app, there's an option to set the --max-old-space-size param. E.g. node index.js --max-old-space-size=8000 - which would set the heap size to 8GB.

I believe setting this param for the spawned process (it is spawned via spawn from node:child_process, right?) could effectively limit the memory used by sandboxed processor...

ppedziwiatr commented 1 year ago

Ok, if I understand properly, they are implemented as forks, (not as spawned processes) -https://github.com/taskforcesh/bullmq/blob/master/src/classes/child-pool.ts#L114

Which is even better, because I believe that each fork has a new V8 instance created = better isolation from the "host" envrionment.

And if that's really the case - it should be possible to set the memory limit per forked process - with sth like:

require('child_process').fork("newProcess.js", { 
        execArgv: ['--max-old-space-size=128']
    });)

https://stackoverflow.com/a/37596117

Would you consider adding the max available memory setting in the Worker configuration options?

manast commented 1 year ago

Yeah well, a fork is also a spawn, the only difference is that it creates a new V8 for you, but it is still a new process so there is no practical difference (with spawn you will need to call the node cmd line tool instead). But sure, we can add support for setting custom flags to child processes, the tricky part is how would such an API look like so that it is consistent with the current design.

ppedziwiatr commented 1 year ago

For me personally - the simplest solution (assuming, that this is more of an "advanced" feature) - with sth like this:

new Worker(updateQueueName, updateProcessor, {
    concurrency: workersConfig.update,
    metrics: {
      maxDataPoints: MetricsTime.ONE_WEEK,
    },
    sandboxExecArgv: ['--max-old-space-size=128', '--foo=bar']
  });

would be awesome!

The sandboxExecArgv would be obviously effectively used only for the sandboxed processors.

ppedziwiatr commented 1 year ago

Hey, any updates? :-)

ppedziwiatr commented 1 year ago

Hey - just asking while reviewing our internal issues ;-) - is there any timeline for implementing this feature?

ppedziwiatr commented 1 year ago

Hey, do you plan to implement this feature?

EDIT: I see, that support for worker_threads has been recently added (https://github.com/taskforcesh/bullmq/commit/0820985e073582fdf841affad38ecc7ab64691ec) - does this new implementation support https://nodejs.org/api/worker_threads.html#workerresourcelimits ?

manast commented 1 year ago

It does not seem to be a lot of interest in this feature. However if someone creates a PR with a clean and simple implementation I do not have anything against merging it.