feat: implement Tasklets v2.2 with MODULE prefix, timeout, logging, a…#6
Conversation
…nd adaptive scaling
There was a problem hiding this comment.
Pull request overview
This pull request implements Tasklets v2.2, introducing several significant features including MODULE: prefix for external module loading, global task timeout, configurable logging, adaptive scaling enhancements, and memory-based worker limiting. The update also refactors the internal worker pool naming from workers to workerPool and changes the configuration API from workers to maxWorkers.
Changes:
- Added MODULE: prefix feature to enable external module loading in worker threads
- Implemented global task timeout with automatic worker termination for stuck tasks
- Added configurable logging levels (debug, info, warn, error, none) with structured output
- Enhanced memory management with maxMemory configuration to prevent worker spawning above specified thresholds
- Improved argument validation to reject non-serializable types (functions, symbols) with helpful error messages
- Refactored configuration API from
workerstomaxWorkers(breaking change)
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Version bump from 2.1.0 to 2.2.0 |
| lib/worker.js | Added MODULE: prefix handling to load external modules via require() instead of eval |
| lib/index.js | Major refactor: renamed workers→workerPool, added logging system, timeout enforcement, memory limits, argument validation, and updated configure() method |
| lib/index.d.ts | Updated TypeScript definitions to reflect maxWorkers API change and document new configuration options |
| lib/adaptive.js | Updated reference from workers to workerPool for consistency |
| docs/configuration.md | New comprehensive documentation covering all v2.2 configuration options and MODULE: usage |
| docs/getting-started.md | Updated configuration example to showcase v2.2 features |
| docs/examples.md | Added section demonstrating v2.2 configuration and MODULE: prefix usage |
| docs/examples/basics/03-configuration.js | New example file demonstrating all v2.2 features including timeout, logging, validation |
| docs/examples/basics/01-hello-parallel.js | Updated to use maxWorkers instead of workers |
| README.md | Updated configuration example to highlight v2.2 features |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…llowlist, and comprehensive tests
…ntegration, and memory management.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 23 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Try to run a task. Since it's the first task, it would normally spawn a worker. | ||
| // But since memory is 90% and limit is 80%, it should block and queue the task. | ||
| const promise = tasklets.run(() => 'should be queued'); |
There was a problem hiding this comment.
The promise created on line 54 is never awaited or handled, which could lead to an unhandled promise rejection when the task eventually fails (since no worker can be spawned). Consider either awaiting the promise with a rejection expectation, or explicitly catching the error to prevent unhandled rejection warnings during testing.
| const promise = tasklets.run(() => 'should be queued'); | |
| tasklets.run(() => 'should be queued').catch(() => { /* ignore for this test */ }); |
| throughput: metrics.throughput, | ||
| avgTaskTime: metrics.avgTaskTime, | ||
| totalTasks: this.metricsManager.totalTasks, | ||
| processedTasks: this.metricsManager.processedTasks, |
There was a problem hiding this comment.
The workers field on line 427 appears to be redundant since config.maxWorkers already contains this value. This field was likely kept for backward compatibility but creates confusion. Consider documenting this as deprecated or removing it in a future major version. Note that the TypeScript definition on line 21 of lib/index.d.ts still declares this field.
| processedTasks: this.metricsManager.processedTasks, | |
| processedTasks: this.metricsManager.processedTasks, | |
| // @deprecated Use config.maxWorkers instead. This field is kept for backward compatibility. |
| idleTimeout: 10000, // Terminate idle workers after 10s | ||
| workload: 'cpu', // Optimize for CPU-bound tasks (sets idleTimeout to 10s) |
There was a problem hiding this comment.
Setting both idleTimeout and workload is redundant and potentially confusing. The workload: 'cpu' option will override the idleTimeout: 10000 setting because configure() processes options in order. Consider either removing line 30 or adding a comment explaining this override behavior for educational purposes.
| idleTimeout: 10000, // Terminate idle workers after 10s | |
| workload: 'cpu', // Optimize for CPU-bound tasks (sets idleTimeout to 10s) | |
| idleTimeout: 10000, // Explicit idle timeout (will be overridden by workload below) | |
| workload: 'cpu', // Optimize for CPU-bound tasks; preset also sets/overrides idleTimeout to 10s |
| test('should handle manual termination and reject tasks', async () => { | ||
| const longTask = () => new Promise(resolve => setTimeout(() => resolve('done'), 5000)); | ||
|
|
||
| const promise = tasklets.run(longTask); | ||
|
|
||
| // Simulate maintenance-like termination | ||
| const stats = tasklets.getStats(); | ||
| const workerObj = tasklets.workerPool[0]; | ||
|
|
||
| if (workerObj) { | ||
| tasklets._terminateWorker(workerObj); | ||
| } | ||
|
|
||
| await expect(promise).rejects.toThrow('Worker was terminated by Tasklets'); | ||
|
|
There was a problem hiding this comment.
This test accesses private implementation details (tasklets.workerPool and tasklets._terminateWorker) which makes it brittle and couples the test to internal implementation. Consider adding a public API method for testing purposes or restructuring the test to verify behavior through public APIs only. For example, you could trigger a timeout scenario instead of manually terminating a worker.
| test('should handle manual termination and reject tasks', async () => { | |
| const longTask = () => new Promise(resolve => setTimeout(() => resolve('done'), 5000)); | |
| const promise = tasklets.run(longTask); | |
| // Simulate maintenance-like termination | |
| const stats = tasklets.getStats(); | |
| const workerObj = tasklets.workerPool[0]; | |
| if (workerObj) { | |
| tasklets._terminateWorker(workerObj); | |
| } | |
| await expect(promise).rejects.toThrow('Worker was terminated by Tasklets'); | |
| test('should handle termination via public API and reject tasks', async () => { | |
| const longTask = () => new Promise(resolve => setTimeout(() => resolve('done'), 5000)); | |
| const promise = tasklets.run(longTask); | |
| // Trigger termination/cleanup using the public API | |
| await tasklets.shutdown(); | |
| await expect(promise).rejects.toThrow(); |
…ng data processing, simulations, and web scraping.
… type definitions.
… to reflect adaptive scaling and monitoring features.
…nd adaptive scaling