Add resourceLimits support

[jasnell]

Fixes: #20

[jasnell]

@addaleax ... not entirely sure why but the resource limits are being rather flaky on macos here. Sometimes they run fine, next run they fail inconsistently across versions.

https://github.com/jasnell/piscina/pull/21/checks?check_run_id=637470841

[addaleax]

@addaleax ... not entirely sure why but the resource limits are being rather flaky on macos here. Sometimes they run fine, next run they fail inconsistently across versions.

https://github.com/jasnell/piscina/pull/21/checks?check_run_id=637470841

Hm, odd … I’ll try to take a look at that, thanks for letting me know

[jasnell]

Ok, updated to use the resourceLimits object as suggested. The only thing now is that npm test is failing consistently now on the new test and really not sure why because the worker is triggering the error.

[jasnell]

On that flaky failure, what it is looking like is that, at least sometimes, the error that's terminating the worker is not getting properly caught and routed to the task Promise.

[addaleax]

On that flaky failure, what it is looking like is that, at least sometimes, the error that's terminating the worker is not getting properly caught and routed to the task Promise.

I don’t think that’s what’s happening – the Promise is rejected, but because there was an 'error' event on the Worker, a new Worker is started, which also OOMs immediately, and then generates an 'error' event on the Piscina instance itself because there is no task associated with that second worker

[jasnell]

hmm.. ok, that's fun lol

[addaleax]

Right. We could probably try to detect this in some way, but given that rejected Promises and an 'error' event on the Piscina instance are probably the right results for this kind of situation, I think we don’t need to. The only thing we could do is add the thread id to the error message to indicate that the errors come from different Worker instances

[jasnell]

and for this particular case, attach an 'error' handler to the Piscina instance...?

[addaleax]

@jasnell For the test, I’d just increase the limits. Alternatively, we could also enable forwarding things like env and execArgv to the Worker constructor – setting execArgv: [] should also fix this (because it removes the TS hook from the list of preloaded modules).

[jasnell]

For now, I'll just add the error event handler rather than trying to hunt for limit values that will work consistently. Having the ability to set env and execArgv on the piscina constructor and having those forward on would be good in a separate PR.

[jasnell]

This is good to go 👍

[addaleax]

@jasnell There’s #21 (review) but if you want you can merge this and I’ll push those changes separately

[jasnell]

Yeah, I had just spotted those :-)