feat: add AbortSignal support at the task level #364
Conversation
|
looks like a great change to me! i'd really love to ditch p-limit, its almost certainly the source of a whole bunch of garbage collection and will be contributing to your memory leak findings. we should be using our own concurrency function given we're a benchmarking library IMO (one we can control and... write better). what do you think @jerome-benoit ? in a separate PR some time edit: have opened #365 and it doesn't seem to hit the same OOM exception when i try it locally |
commit: |
|
The feature implementation looks good. Please address the typing issue in tests triggering a CI failure. Thanks. |
Fixes tinylibs#139 This change enables `Task` to be aborted via an `AbortSignal` which falls back to a signal its `Bench`'s signal. It adds a `signal` prop to the `Task` class, as well as an `isAborted()` method (which could be a getter, if you wish). I'm wrapping `tinybench` and have confirmed this works well, as I'm able to abort (async) tasks via the signal. I've added tests covering intended usage and some edge cases, but it seems there may be a memory-leak issue with concurrent tasks (I'm not sure if this is a known bug); this test (`task-level abort: works with task concurrency`) is skipped. If you can get it working, then maybe it's just my environment. I added documentation about the bench-and-task-level-`AbortSignal` support to `README.md`. Happy to remove if it's not needed--or I can remove the examples, or whatever.
boneskull
force-pushed
the
boneskull/issue-139
branch
from
419b959 to
e1aff20
Compare
|
OK, fixed the type error. Thanks for looking at this so quickly! |
jerome-benoit
left a comment
jerome-benoit
left a comment
There was a problem hiding this comment.
Not a single nitpicking review comments to add ;)
3 participants
Fixes #139
This change enables
Taskto be aborted via anAbortSignalwhich falls back to a signal itsBench's signal.It adds a
signalprop to theTaskclass, as well as anisAborted()method (which could be agetter, if you wish).
I'm wrapping
tinybenchand have confirmed this works well, as I'm able to abort (async) tasks viathe signal.
I've added tests covering intended usage and some edge cases, but it seems there may be a
memory-leak issue with concurrent tasks (I'm not sure if this is a known bug); this test
(
task-level abort: works with task concurrency) is skipped. If you can get it working, thenmaybe it's just my environment.
I added documentation about the bench-and-task-level-
AbortSignalsupport toREADME.md. Happyto remove if it's not needed--or I can remove the examples, or whatever.