failsafe-lib / failsafe

Fault tolerance and resilience patterns for the JVM
https://failsafe.dev
Apache License 2.0
4.16k stars 295 forks source link

ExecutionContext#getStartTime() returns Duration #347

Closed brainbytes42 closed 1 year ago

brainbytes42 commented 1 year ago

https://github.com/failsafe-lib/failsafe/blob/55b17b249d1a4498d29dff4038bd1aa9e7db155b/core/src/main/java/dev/failsafe/ExecutionContext.java#L83

Is it intentionally to return Duration instead of Instant?

From the docs I'd assume the getElapsedTime() would be the Duration between the getStartTime()-"should-be-Instant" and now...? Or what's the difference otherwise?

-- footnote: just going through 3.x-changes, looks great so far! (was still on 2.x-version...) :-)

jhalterman commented 1 year ago

Thanks for bringing this up. getElapsedTime() works as you say, here's the implementation. But getStartTime is indeed problematic since it uses System.nanoTime internally, which returns nanoseconds since some arbitrary start time. You're right that returning an Instant here would be more appropriate.

There's a few deprecated methods in the API that I've been waiting to remove. I may just cut a 3.3 release to do that and make this change at the same time. Normally I'd prefer to deprecate an API before removing it, but in this case it's really a change, not a deprecation, that we want. An alternative would be for the returned Duration to be based on System.currTimeMillis, but returning a Duration here was just never a good idea.

brainbytes42 commented 1 year ago

nice, that was a quick fix! 👍

jhalterman commented 1 year ago

I am actually wondering if getStartTime should return Optional<Instant> since the start time will be null if an execution is cancelled before it begins (such as if it's waiting on a thread and a timeout occurs). Previously this method was never returning a null.

There are other methods that may return null, such as ExecutionCompletedEvent.getResult or getException, but I wonder if returning null from getStartTime might be more surprising.

Tembrel commented 1 year ago

That makes a lot of sense to me. It's easy to miss that something can be null, but it's impossible not to notice an Optional.

jhalterman commented 1 year ago

Closed again with ed8de6717e8e7222b30163506702cbfd08b99213.

jhalterman commented 1 year ago

This has been released in 3.3.0.