search

josdejong / workerpool

Offload tasks to a pool of workers on node.js and in the browser
Apache License 2.0
2.06k stars 147 forks source link

feat: support cleanup on termination in worker context #377

Closed Michsior14 closed 1 year ago

Michsior14 commented 1 year ago

Adds termination handler in a worker context to allow releasing resources.

Tests will be added after feedback (to confirm interfaces/idea).

Resolves #353

josdejong commented 1 year ago

Thanks, I'll review your PR tomorrow. I see there are no tests added. Would it be possible to unit test the new cleanup functionality?

Michsior14 commented 1 year ago

Thanks, I'll review your PR tomorrow. I see there are no tests added. Would it be possible to unit test the new cleanup functionality?

As in the PR description I wanted to first confirm this is the way to go ;)

josdejong commented 1 year ago

ah sorry, makes sense 😅

josdejong commented 1 year ago

This looks really good @Michsior14, exactly what we need 👌 . If you can add tests I think all is ready to merge this PR.

Michsior14 commented 1 year ago

@josdejong I've added working tests using MessageChannel since I had no other idea how to tackle this.

josdejong commented 1 year ago

Thanks! It is indeed not so trivial to test whether onTerminateWorker since it is shutting down the worker. Sounds like a good solution to use MessageChannel! I'll now merge your PR and publish a new version of workerpool at npm.

josdejong commented 1 year ago

Published now in v6.4.0, thanks again!