connor4312 / cockatiel

🐦 A resilience and fault-handling library. Supports Backoffs, Retries, Circuit Breakers, Timeouts, Bulkhead Isolation, and Fallbacks.
MIT License
1.57k stars 48 forks source link

Allow timers to be unreferenced #13

Closed novemberborn closed 4 years ago

novemberborn commented 4 years ago

By default, unreference timers created by the TimeoutPolicy. Add an option to keep the timer referenced.

Add an option when buliding a RetryPolicy to unreference the timer.

Fixes #11.


Specifying an unref option when creating the policy seems the most natural to me. It's a bit awkward in Policy.timeout() though since that function already takes two positional arguments. Rather than adding a third I've added an optional options object instead. But maybe Policy.timeout() should be changed to take a single options object.

It's hard to test unref(). I could stub setTimeout, or perhaps we can do manual testing. What do you think @connor4312?

novemberborn commented 4 years ago

Cheeky force-push to fix Markdown linting errors (which I need to set up on my own projects, nice!).

connor4312 commented 4 years ago

I'm still not totally clear on the use case for this. It seems dangerous to allow users to execute a call that could be interrupted by program execution, if there happened to be no other tasks running. Would letting you pass a cancellation token to the retry policy (probably a good idea in general) solve the problem you're running into?

novemberborn commented 4 years ago

I'm still not totally clear on the use case for this.

Allow the process to exit gracefully even if a retry is scheduled.

It seems dangerous to allow users to execute a call that could be interrupted by program execution, if there happened to be no other tasks running.

Which is why it's not the default behavior.

But yes I can see how it's very implicit.

Would letting you pass a cancellation token to the retry policy (probably a good idea in general) solve the problem you're running into?

Yes, I still need to cancel the actual execution, for which I'm already using a cancellation token.

I think it makes more sense for the token to be passed in the execution() call. Not sure what the ideal interface is for that, since it currently just takes a function.

Maybe an executeWithCancellation() which returns Promise<[Cancellation, Result]>?

connor4312 commented 4 years ago

Thanks for the contribution, and apologies in the delay in merging it.

I opted to make these builder methods, in the fluent style of the library. And give them clear names so that people think carefully about using them 🙂 For example:

const response1 = await Policy.handleAll()
  .retry() 
  .dangerouslyUnref() 
  .attempts(3)
  .execute(() => getJson('https://example.com'));

As for testing, I compromised between manual testing and unit testing by spawning child processes and checking for termination behavior.

https://github.com/connor4312/cockatiel/blob/1c1871dfa5cc98f6fe5b339791ac0a231f009cae/src/TimeoutPolicy.test.ts#L57-L64

Changes are published in 0.1.5. Let me know if you run into any other problems.