search

failsafe-lib / failsafe

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

Breaking API change in 2.4.2? #289

Closed JimNero009 closed 3 years ago

JimNero009 commented 3 years ago

Hello! I think we stumbled across a breaking issue in your lib today that I thought I'd flag. I'm sure that this has caused a breaking issue, but I'm not 100% sure on my analysis. The issue appears to come from this change: https://github.com/failsafe-lib/failsafe/commit/940f6b74e9400a303e29f4fd0b34f98000ef5a01#diff-6adc71314e3dec09e77e58c7986489e1d8f8aeb24423320884644fd6765acd1c

As you can see, you changed the method signature so that you 'promote' one policy, the first argument, whereas before the list of args were all part of the same '...' argument. I think this can cause issues for projects where this dependency enters the graph in more than one place.

For example, say I have project A that depends on project B, and both of these projects want to depend on you. Let's say project B was compiled with the earlier version of the code, so with the one '...' argument. Let's say both call the 'with' function with just one argument. The Java compiler, as I understand it, changes this under the hood to an array, so the signature is actually translated into a one-arg array function. Now project A depends on the latest version of you, and when that is compiled, the signature that it is translated into isn't a one-arg array function, but a two arg function with a string and what happens to be in this example an empty array. So when code reaches the part where project B calls the function, and project B hasn't packaged the code directly and is resolving out to the latest version of this dependency, the code won't be able to find that signature and things will break.

I hope that makes sense? I think one way of fixing could be to re-add the other constructor and mark it as for deprecation and re-route to the new one so that things remain compatible until it's cleared up in a major release.

WDYT?

Tembrel commented 3 years ago

Adding back the with(Policy.... policies) method, even deprecated, would make for ambiguity:

    Failsafe.with(retryPolicy)    // ambiguous: with(P...) or with(P p, P... ps)?
JimNero009 commented 3 years ago

Good point.

Is there a good way out of this then? I do have access to the troublesome libs for my particular case, but I’m a bit concerned about how to deal with this in a general case.

jhalterman commented 3 years ago

@JimNero009 Apologies, that kind of binary incompatibility was not intended. The intent of that commit was to, essentially, always require at least 1 policy when using Failsafe.with, which always should have been the case.

We could revert the change and cut a new patch release, waiting for the next minor or major to re-introduce it. I wonder though if that just reverses the problem, where 2.4.2 users would then have their own binary incompatibility (I can investigate this more later...)

JimNero009 commented 3 years ago

Not a problem, these things happen and it took me in association with some others the good part of a day to finally realise what had happened! Sadly I think you’re right in that the keen upgraders will be caught out in any revert process.

As is stands right now, I’ve no better suggestions but I will try and think about some ways we can alleviate the problem. I raised this issue mostly in the hope that others that hit the issue can get to the root cause more efficiently than we did :D

jhalterman commented 3 years ago

I suspect this issue may become more widespread, so reverting the commit and cutting another patch release could be the best course of action.

jhalterman commented 3 years ago

@Tembrel I think something like this would avoid the ambiguity problem, and effectively rollback the old method removal while still keeping the new method: 73b3c40826f330e91c24dcaa42e122d78712c5b5.

I marked added Failsafe.with(Policy[]) and marked it as deprecated, FWIW, but when any new code is compiled against the patch release, it would prefer Failsafe.with(Policy, Policy...).

Tembrel commented 3 years ago

Is the signature of varargs with(P... ps) the same as with(P[] ps)? Or more specifically, will code compiled against the former link with the latter?

jhalterman commented 3 years ago

Yea, that seems to be the case and it works from my testing.

jhalterman commented 3 years ago

Fixed by 821122f80131b69be9f7b4b0161716a5b39b22de.

jhalterman commented 3 years ago

@JimNero009 2.4.3 was just released with this fix. Thanks for reporting this!

JimNero009 commented 3 years ago

Advantage of a global system! Went to bed in the UK, when I get up, you've solved the issue and patched -- nice!

Can confirm now that 2.4.3 does indeed appear to solve the issue. Thanks for the quick work on this, and keep being awesome.