connor4312 / cockatiel

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

TimeoutPolicy not firing events onTimeout nor onSuccess #27

Closed nicodeg87 closed 4 years ago

nicodeg87 commented 4 years ago

Hi, first of all, thanks a lot for this amazing library.

I'm having problems with Timeout policy, please see the following code snippet:

const timeout = Policy.timeout(1000, TimeoutStrategy.Aggressive);

timeout.onTimeout(({ duration }) => { console.log(`onTimeout. ${duration}ms`)});
timeout.onSuccess(({ duration }) => { console.log(`onSuccess. ${duration}ms`)});
timeout.onFailure(({ duration, handled, reason }) => { console.log('onFailure', handled, reason)});

async function delayedResult(duration, result = true) {
    await new Promise(r => setTimeout(r, duration));
    return result;
}

//....
result = await timeout.execute(async () => await delayedResult(5000, {success: true}));

I see two issues with the current implementation:

1- Having 5 seconds of execution time with 1 second of timeout:

Expected Result: We should get fired onTimeout event. Actual Result: No onTimeout event is fired. We get the exception TaskCancelledError which is ok but after the 5 seconds past we get the onFailure event call which according docs this event should not be called on timeout scenario.

2- Having 1 second of execution time with 3 second of timeout:

Expected Result: We should get fired onSuccess event as there is no timeout condition. Actual Result: We get fired onFailure event. It seems to be related to the default resultFilter used on the ExecuteWrapper class.

connor4312 commented 4 years ago

Thanks for the excellent issue, fixed in version 1.1.1

nicodeg87 commented 4 years ago

Hi Connor,

Thanks a lot for the quick fix, it's working perfect now :).

Just one quick comment: the documentation needs to be fixed according the new behaviour of firing failure event when the timeout occurs. On this Section: [https://github.com/connor4312/cockatiel#timeoutonfailurecallback], the following comments needs to be removed (or changed):

"This is only called when the function itself fails, and not when a timeout happens."

Again, thanks for the quick actions.