RetryPolicy thread safety

[failsafe-lib]

RetryPolicy thread safety

[Tembrel]

I'm sorry I didn't notice that I was tagged back in October.

Initializing both condition lists with CopyOnWriteArrayList would avoid the CME between RetryPolicy#retryWhen and RetryPolicy#canRetryFor and similar situations, but there are other safety problems with sharing instances between threads: The double fields are vulnerable to out-of-thin-air values and the visibility of other fields is not guaranteed. Making all fields volatile (except the condition lists, which should be final) would provide visibility guarantees, but even then the non-atomicity of methods like withBackoff that modify several fields would still leave RetryPolicy fundamentally thread-unsafe.

You could slap the @ThreadUnsafe annotation on the class, but documenting safe uses of a thread-unsafe class is tricky. The only safe way to share is to copy before sharing to a different thread:

    RetryPolicy orig = ...;
    RetryPolicy copy = orig.copy();
    // ok to use orig and copy here (see correction note below) [1]
    CompletableFuture<Result> f = CompletableFuture.supplyAsync(() -> {
        // all use of copy should be in here, guarded by
        // implicit happens-before edge
    });
    // don't use copy here, ok to use orig [2]
    Result result = f.join();
    // safe to use copy here: happens-before edge due to join()

No one should have to perform this kind of delicate reasoning when using a class, so I think documented thread-unsafety is not a good answer.

The options, then, are either to make the class thread-safe mutable or to make it immutable, as @akincel noted. Unless someone can give compelling use cases for mutability in response to @jhalterman's request, immutability is clearly preferable. The bar is high: To anyone thinking of presenting such a use case, I would ask whether they wouldn't be able to make do with a MutableRetryPolicyHolder class with a mutable reference to an (immutable) RetryPolicy.

I don't think the cost of creating a new RetryPolicy instance for each "mutable" method call is particularly high in practice. It's hard to imagine uses that involve creating instances in a tight inner loop, because the RetryPolicy is designed to be applied to repeatable tasks that typically take some time.

Bottom line: @Immutable is the way to go.

Very late correction (2020-July-9)

I got this one (the line marked [1]) wrong initially: There is a happens-before edge between actions taking place before submission of a task (which is what supplyAsync does) and the execution of that task. It's still the case that you can't use copy at [2].

The fact that I got this wrong just goes to show how tricky the reasoning around thread-unsafe objects can be.

[reutsharabani]

Sure.

I'm not going to go into very specific details but generally I have a service that constructs the default RetryPolicy which I'm later using for several other services with a little different retry patterns. Each of these services builds on top of that base configuration.

The configurations are being accessed from different threads so changes that are made (without copying the instance first) are shared to all other threads obviously.

This can be a problem with mutable objects such as ArrayList or Duration, and currently I'm copying the configurations for each access pattern (but it is easy to forget IMO - cause nothing prevents me from mutating objects inside RetryPolicy).

So, I can resolve this with forcing immutability (force copy to change), which will make my code much nicer and easier to maintain.

I've actually made a fork of this repository to start using the builder pattern with immutability to resolve this for myself. This is sort of how you do it, but your way keeps the RetryPolicy mutable (which is probably fine most of the time, just not what I need).

Currently two of your tests fail, but I didn't go much into them. I'll probably publish it later on so you can see what I did.

• It's a breaking change obviously, that's why I asked if there is a standard way of doing these things built in to the library other than RetryPolicy.copy()-ing everything and relying on the user.

Thanks for the quick reply!

[jhalterman]

@reutsharabani From your scenario, it sounds like you need different RetryPolicy instances for each service no matter what since their retry patterns will vary. These can certainly be copied from a common base policy via copy(), but it's not clear that thread-safety will solve anything in the scenario you described.

Thread-safety would only be an issue if you were configuring a single RetryPolicy from multiple threads concurrently, or configuring a RetryPolicy while using it via a Failsafe call. Is there a scenario where you need to do something like this?

@sunng87 RetryPolicy is certainly reusable and shareable. Failsafe will never modify a RetryPolicy, only read them. That said, since RetryPolicy is currently not thread-safe, unexpected behavior could result if reading/writing to it concurrently.

[sunng87]

I thought RetryPolicy is reusable when designing diehard API. Now I do policy.copy() internally. However, I still think it would be better to separate the mutable part and make this configuration object immutable.

[ben-manes]

I dislike (1) if multiple distinct settings are being modified without a transactional style api. If using policy.abc(0).xyz(5) then another thread might see partial updates from the distinct calls. You would have to have a transactional api to composable the modifications, e.g. policy.update(config -> ...) which isn't attractive. Then to make it fast you might use a versioning flag (ala StampedLock) to validate reads. Just a mess.

I prefer immutability and favor the builder pattern as the most obvious code. I'm not super concerned about the GC since the intermediate objects are short lived and the work being managed must be comparably expensive.

[jhalterman]

@reutsharabani So you're just suggesting that the API should be made thread-safe in order to prevent users from doing the wrong thing? This suggestion could apply to anything that is not thread-safe, but is fair enough.

We could make a configuration/builder for creating RetryPolicy instances, but you'd still need to build a new instance each time you want to change something, which is the same effort as making a copy(). I don't like the copy-on-write approach that Clojure uses for its persistent data structures here, because it would only benefit a very small number of users or use cases.

[akincel]

@jhalterman I would say it was mostly documentation issue. The scenario I described a few months ago was modifying the configuration on the fly (If I remember correctly). That sounds more like wrong usage because documentation is/was not clearly stating what's the intention there.

Making RetryPolicy immutable could be also one way how to prevent such behaviour.

[whiskeysierra]

the copy-on-write approach that Clojure uses for its persistent data structures

Not relevant to the discussion, but that statement is not a fair assessment of clojure. Persistent data structures in clojure are not copy-on-write. Any modification will give you a new value, but it's not just a full copy as the term copy-on-write suggests. It's rather re-using 99% of the structure of the previous version, making this both fast and extremely memory-efficient.

This approach would work for RetryPolicy, but it won't give any memory savings, since all the state within retry policy is primitive in nature. That said, I'd still prefer it over a mutable configuration object.

[whiskeysierra]

Why not separate the configuration from the result and make both immutable?

[Tembrel]

Thread safety can be achieved through a variety of techniques, depending on the specifics of the state being shared and how it is accessed. Synchronization is the most commonly used technique, but it isn't sufficient for all situations. I haven't done an analysis of the most recent code to know what the best approach would be. But this is why I urge immutability where possible. It avoids the need for tricky reasoning about thread safety.

…

On Fri, Aug 16, 2019, 2:28 PM Willi Schönborn ***@***.***> wrote: internally threadsafe Synchronized? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#47>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABZ5SS3DRUCQU5YUHQF3FDQE3WVLANCNFSM4CLSDCKA> .

[whiskeysierra]

But once you have immutable objects it's safe to promote reusing, caching and statically declaring them.

[Tembrel]

It looks like there are 19 fields in RetryPolicy: 14 in the class itself, 3 in DelayablePolicy, and 2 in FailurePolicy. Two of these are lists that would typically have a few entries each, but it would be unusual to have an order of magnitude more. So creating a new instance for every method call in the configuration chain, while superficially wasteful, is very likely to be inexpensive relative to the actual execution that a RetryPolicy is used for. And these days the JIT/runtime should be pretty good about limiting overhead.

I could be wrong about this — I'm stuck on Java 8 for annoying reasons, so I haven't been able to play with the latest stuff, but it's hard for me to imagine usage that would be a problem in practice with even the simplest of immutability schemes. It would have to be a retryable action whose likely cost was substantially more than the cost of allocating and configuring of one instance, but also substantially less than, say, 6 times that allocation/configuration cost. And one always has the option of not creating the RetryPolicy on the fly.

CircuitBreaker is a different story, unfortunately, because the execution state fields (state, currentExecutions) are intermingled with the configuration state fields (everything else). That's why a builder pattern makes sense there: an immutable class to represent the configuration, and a mutable (though still necessarily thread-safe) class to hold the shared circuit breaker state fields.

[jhalterman]

Waking up this thread after a while :)

Re: this comment from @Tembrel

CircuitBreaker is a different story, unfortunately, because the execution state fields (state, currentExecutions) are intermingled with the configuration state fields (everything else). That's why a builder pattern makes sense there: an immutable class to represent the configuration, and a mutable (though still necessarily thread-safe) class to hold the shared circuit breaker state fields.

They are indeed intermingled. I leaned towards making policies quick and easy to construct, where no additional objects are needed for configuration, registering event listeners, or retrieving metrics. Everything you need is within the policy object itself. In hindsight, I don't think that was a bad idea, but clearly I underestimated the need for threadsafe policy configuration for some users.

I don't mind introducing builders for policies (though this might be a breaking change requiring a major version bump), but part of what makes this challenging is that they don't necessarily make sense for all policies...

In the case of CircuitBreaker, we actually want the policy to be mutable since they are meant to be shared throughout a codebase and replacing references could be difficult for users. It could still have a builder for the initial construction, but I suspect we'd want reconfiguration methods as well, since being able to tweak the thresholds for a CircuitBreaker on the fly is a useful feature. Ex:

CircuitBreaker<Object> cb = CircuitBreaker.builder().withFailureThreshold(3, 10).build();
cb.setFailureThreshold(1, 3); // mutable, but threadsafe

Of course, if you're going to have method for mutation then the builder doesn't seem to be needed.

RetryPolicy could just require builders, where additional mutation would require going through a new builder, and that seems ok. Ex:

// Build a new RetryPolicy from scratch, based on defaults
RetryPolicy<Object> rp1 = RetryPolicy.builder().withMaxRetries(5).build();

// Build a new RetryPolicy based on an existing one
RetryPolicy<Object> rp2 = rp1.builder().withDelay(Duration.ofMillis(100)).build();

Timeout only has one optional setting atm, so it's debatable whether a builder is useful:

Timeout<Object> timeout = Timeout.builder(Duration.ofMillis(100)).withInterrupt(true).build();

Fallback only has one argument which is required, so it doesn't seem worthy of a builder for now, but one could be added in the future.

[jhalterman]

3.0 has been released, which includes this improvement.

[jhalterman]

RetryPolicy is currently not threadsafe. Since it's basically a configuration class that you'd probably only ever use from one thread, it didn't seem necessary to make it threadsafe.

If you have use cases that require thread safety in RetryPolicy, can you tell me a bit about them?

[reutsharabani]

... or configuring a RetryPolicy while using it via a Failsafe call. Is there a scenario where you need to do something like this?

I don't need to do it, but the API provided does not prevent me from doing so. I already mentioned I share the base configuration instance by copying and then mutating it and offered a way to change the API to actually enforce it by separating configuration (instance builder) from initialization (instance).

So no, I don't need to do it, it's just that the current API reflects that I may be able to (that's why I asked).

And true, thread safety is actually just a by-product of immutability in this case, so the point is really about making configuration instances more shareable and removing the user's responsibility of remembering to copy and then mutate.

This way you can safely initialize anything with your retry configuration and not care about someone mutating it or copying when it when passing it along or when allowing access to it (via getter...), since mutating it not possible.

I also understand the other comment here since with a language like clojure, immutability is the default way of doing things. To me it makes sense here as well.

[marcreichman]

I realize this is long out of date, but if I wanted to setup a static final shared instance of a RetryPolicy with no changes, could I use it from multiple Failsafe.with(retryPolicy).. calls at the same time? i.e. when Failsafe is executing using a retrypolicy, is it maintaining state with the policy? or within Failsafe itself?

e.g. is this OK?

private static final RetryPolicy MY_SHARED_POLICY = new RetryPolicy()
            .retryOn(ProcessingException.class)
            .withMaxRetries(3)
            .withBackoff(1, 3, TimeUnit.SECONDS)
            .withJitter(0.25d);

// called from multiple threads
public void doMyThing() {
  Failsafe.with(MY_SHARED_POLICY).get(MyClass::myFunction);
}

[jhalterman]

@marcreichman Perfectly safe to share retry policies, and encouraged, as long as you're not modifying the retry policy while it's in use (in that case, make a copy()). Failsafe itself never modifies the state of a retry policy.

[MALPI]

@jhalterman I am not sure about that. We are also reusing retry policies between threads but figured out today that we run into a bug there which is trigger ConcurrentModificationException on

net.jodah.failsafe.RetryPolicy.canRetryFor(RetryPolicy.java:175
net.jodah.failsafe.AbstractExecution.complete(AbstractExecution.java:119)

although we don't modify the policy at all.

As side effect the Http-Connection is left open.

[jhalterman]

@MALPI If there's a bug here, I definitely want to find it. Any chance you can provide a simple reproducer for the ConcurrentModificationException you're seeing?

[MALPI]

@jhalterman I will try to create an example during the next days.

[aaylward]

@MALPI any progress?

[MALPI]

Sorry I simply forgot about that. @akincel so you remember?

[akincel]

We experienced the ConcurrentModificationException when modifying the RetryPolicy configuration by adding predicate to retryConditions in one thread while in other thread RetryPolicy was checking if retry should happen or not.

In code, concurrently executing:

• RetryPolicy#retryWhen
• RetryPolicy#canRetryFor

It is already discussed above, that it is not totally clear from documentation and the RetryPolicy is not immutable, therefore it is easy to produce such coding/logic errors.

[jhalterman]

@akincel (I know it's been a while since you filed this) - curious about your use case for modifying a RetryPolicy that is in use versus creating a .copy(). Is this just a documentation issue or do you have a good use case for wanting to modify a RetryPolicy that is in use? Any description on that is appreciated.

I'm cool with making RetryPolicy threadsafe if necessary, but I hadn't seen a good use case for it yet and wanted to avoid the overhead if possible.

@Tembrel - Any thoughts here?

[Tembrel]

But if people write in to say that, in defiance of the chaining pattern established in the Failsafe README, they are actually relying on code like this:

    void configureRetryPolicy(RetryPolicy rp) {
        rp.withMaxRetries(3);
    }

    void someOtherCode() {
        RetryPolicy rp = new RetryPolicy();
        configureRetryPolicy(rp);
        // rp.maxRetries == 3
    }

then a hybrid scheme, where RetryPolicy is the thread-safe, mutable base class, and RetryPolicy.immutable() produces an instance of subclass ImmutableRetryPolicy, would be possible.

In that case, the questionable fragment above would work as the user expected, but the following would not:

    RetryPolicy rp = RetryPolicy.immutable(); // reasonable defaults
    configureRetryPolicy(rp);
    // rp.getClass() is ImmutableRetryPolicy.class, unchanged by method call

That sort of structure is tricky to get right, so I really hope no one is relying on mutability in this way.

[fante76]

Hi to all
I found a problem like @MALPI. I built class with a static RetryPolicy object

private static final RetryPolicy stdPolicy = new RetryPolicy().withMaxRetries(5).withBackoff(1, 10, TimeUnit.SECONDS).withJitter(0.2)

The class is part of a web J2EE application, so it can be used by multiple threads. Such way results in IOException after the very first use of that policy. The reason for this is to use same policy for all class methods.
Instead, if I create a new RetryPolicy every time I ask for it, it works fine. It's not blocking, but I hope this can add informations to the resolution.

[jhalterman]

Picking up on @Tembrel's idea here, we could offer policies that are threadsafe (via locks) by default but that offer an optional .copyOnWrite() method that wraps the policy in a variant that copies on each write and where the internal lock becomes a noop lock. The would allow users to optimize for reads or writes: the locking policies would be more optimized for writes, the copyOnWrite policies would be more optimized for reads.

[Tembrel]

Maybe I'm missing something, but I don't see a downside to immutability for policies (and maybe for FailsafeExecutors?).

[jhalterman]

Why not separate the configuration from the result and make both immutable?

So something like a policy builder?

RetryPolicy.builder().handle(FooException.class).build();

Edit: nvm, I re-read @Tembrel's comment here re: immutable(). I kind of like that better than the builder approach (which is a bit verbose). That still doesn't completely alleviate thread safety concerns for users who don't use the immutable variant though.

[Tembrel]

I was thinking that each configuration method just creates a new immutable instance, no need for separate build step.

[jhalterman]

I was thinking that each configuration method just creates a new immutable instance, no need for separate build step.

So basically RetryPolicy would be copy on write by default?

Edit: It might be nice to offer locking and copyOnWrite variants if that's reasonably easy to do internally. If we did that, it's just a matter of picking what the default should be.

[whiskeysierra]

For retry that's pretty safe. With circuit breaker it's somewhat risky if each step is a valid circuit breaker with state because if different parts of your system believe to use the same instance but actually use similar yet independent copies then the behaviour will be surprising.

[Tembrel]

That's true. Maybe a builder pattern for CircuitBreaker makes more sense then.

…

On Fri, Aug 16, 2019, 1:59 PM Willi Schönborn ***@***.***> wrote: For retry that's pretty safe. With circuit breaker it's somewhat risky if each step is a valid circuit breaker with state because if different parts of your system believe to use the same instance but actually use similar yet independent copies then the behaviour will be surprising. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#47>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABZ5SVMP4SLIOES4IKFMH3QE3TH5ANCNFSM4CLSDCKA> .

[jhalterman]

Any reason why we shouldn't just make CircuitBreaker and RetryPolicy internally threadsafe? To mitigate the cost of threadsafe reads, the PolicyExecutors could read and store Policy settings when they're constructed, that way if they get called multiple times we avoid the hit of the threadsafe read (which is likely not an issue for most users/use cases anyways).

[whiskeysierra]

internally threadsafe

Synchronized?

[Tembrel]

@jhalterman is your concern that implementing immutability in the simplest way would result in a lot of unused intermediate copies of policy instances?

[jhalterman]

Yea, immutability might create a lot of object allocations if people are creating lots of policies on the fly rather than reusing them. And thread safety, while it shouldn't be too bad to implement (the policies aren't that complex), will have a bit of a cost when a PolicyExecutor needs to read some policy configuration.

[jhalterman]

@ben-manes I'm curious if you have any thoughts on how best to make RetryPolicy and CircuitBreaker threadsafe from a usability perspective? Overall we've discussed a few options:

1. Use internal locks/synchronization
   a. Optimized for policy writes, more expensive reads
2. Make them copy on write
   a. Optimized for reads, more expensive writes
3. Make them immutable and only buildable via a builder
   a. Bulkier API, possible breaking changes
4. Use internal locks by default (option 1) but support an optional copyOnWrite() method that adapts a policy to use copy on write internally.
   a. Best of both worlds with options 1 and 2?

[jhalterman]

I dislike (1) if multiple distinct settings are being modified without a transactional style api.

Would something like that realistically be an issue for a policy though?

I can see the support for doing a builder, though that's the approach I'm least excited about since it will (probably) mean another API breakage and impact usability for simple use cases:

// Current
new RetryPolicy<Boolean>().handleResult(false).withMaxRetries(2);

// With builder approach
RetryPolicy.<Boolean>builder().handleResult(false).withMaxRetries(2).build();

// With config approach
new RetryPolicy<Boolean>(
  new RetryPolicyConfig<Boolean>().handleResult(false).withMaxRetries(2)
);

[ben-manes]

Why allow concurrent modifications at all? If you do not want to modify the API for compatibility and usability concerns, you could freeze the configuration on first use and fail if changes are tried afterwards. It sounds like that would best fit your intent.

[timothybasanov]

Hey, I've just noticed that RetryPolicy is not thread safe as of 2.4.0. One example:
delay field is not volatile, nor final, there is no synchronization around the write in the constructor.
This means that some basic operations may be broken, this is definitely not what the user may expect:

• When one thread creates it and stores in a static field, and another does retryPolicy.getDelay().toMillis() it may throw NPE (unsafe publication)
• If one thread uses FailsafeExecutor and another thread calls withDelay then this change may never(!) reach the execution thread (no happens-before)

Could you at least mark all the policies with @NotThreadSafe to show that I have to synchronize any access to the policy myself? Note that I'm not sure if it's even possible to correctly synchronize outside of the library as library calls itself and these calls can not be synchronized from the outside.

Personal opinion: While we can talk about different solutions on how to make the library thread safe, I believe that thread safety is P0 for any retry library. If it's not safe I would not use it, period. Even if it's fast.

This is not a contribution

[Tembrel]

Hey, I've just noticed that RetryPolicy is not thread safe as of 2.4.0.
Personal opinion: While we can talk about different solutions on how to make the library thread safe, I believe that thread safety is P0 for any retry library. If it's not safe I would not use it, period. Even if it's fast.

In my comment earlier in this issue and subsequent comments on this and other issues, I advocate for immutable policies, using withXXX methods to create new instances. (I also made an argument that this can be done efficiently.)

In this comment I argue that although doing so might break existing code that relies on the mutability that currently prevents the code from being thread safe, nobody should be using such code.

[timothybasanov]

As a user of library I would be surprised with significant non-backwards compatible changes.
I think the least surprise would be to release a hot-fix that adds synchronization everywhere (at a cost of a performance impact.)
I'd say that a major version bump would be required to make non-compatible changes like you propose (which I do whole-heartedly support btw.)

This is not a contribution.

[Tembrel]

I recommend against synchronizing everywhere. The performance concerns are minor, but over-synchronization can lead to liveness problems, including deadlock.

• When one thread creates it and stores in a static field, and another does retryPolicy.getDelay().toMillis() it may throw NPE (unsafe publication)

Unsafe publication is a problem in any case. Any user writing values to static fields that will be shared between threads needs to take precautions regardless of whether there are unguarded fields. At a minimum, a static field used in this way could be volatile, to provide a happens-before edge between volatile write and volatile read. (Even a fully immutable instance wouldn't have a visibility guarantee otherwise.)

• If one thread uses FailsafeExecutor and another thread calls withDelay then this change may never(!) reach the execution thread (no happens-before)

That was the point of this comment. Any code that tries to share retry policies between threads and mutate them after initial configuration is asking for trouble. I'm in favor of documenting this, perhaps with @NotThreadSafe, but also more helpfully with some guidance on best practices in the documentation. Something like:

Don't rely on side effects; use the result of a fluent method invocation instead of its target. Perform all policy configuration before passing to Failsafe.with(...), and use copy() to create new instances that can then be configured differently.

Code that follows this advice would continue to work without change under the immutability proposal, albeit with superfluous calls to copy(), which would be a no-op: return this;

As a user of library I would be surprised with significant non-backwards compatible changes.

I'd rather be surprised by changes that discourage unsafe usage of Failsafe than lulled into complacency by piecemeal thread-safety tweaks that continue to allow risky usage.

But yes, it would probably mean a major version bump. I wish I had been able to push more forcefully for immutability after my first comment on this issue, nearly three years (!) ago.

[timothybasanov]

TL;DR: I agree with your arguments, Tim. I still think that in this specific case we still should synchronize.

In general synchronization may cause live- and deadlocks, but here the proposal is only to synchronize on getters/setters for a policy. This is safe as we don't make any downstream calls. If we can do a proper read/write lock it would be even better for read contention. The only downside—increase in code verbosity.

Yep, I finally grasp your argument on safety of publication. Policy itself has a relation with all the executions it starts. If one thread creates an instance of a policy it has to provide HB relationship to the second thread anyway. Any attempt to mutate a policy anywhere in between is bound to fail. I still think it's a good idea to make policies more resilient even in the event of unsafe publication as "defense in-depth" strategy. Copy on write or builder pattern policies would be ideal, copy() is a good alternative in the mean time.

Fully support update of the JavaDoc, seemingly this would be the safest and simplest way to at least partially address the situation. Three years seems like a very long time. Hopefully we'll see some changes in 2020! :)

This is not a contribution

[Tembrel]

I still think that in this specific case we still should synchronize.

In general synchronization may cause live- and deadlocks, but here the proposal is only to synchronize on getters/setters for a policy. This is safe as we don't make any downstream calls.

Probably, but over-synchronization is a bad code smell, and it still wouldn't fix all thread-safety issues. For example, the abortConditions field is initialized with ArrayList.

It's very hard to get thread-safety right, and in this case doing so would only be in support of usage that is inherently risky and ill-defined. Even with full thread-safety, what would it mean for separate threads to modify a retry policy concurrently?

I think the best thing for now is to promote a safe pattern of use, as @jhalterman's comment in this issue hints and as we've discussed.

Hopefully we'll see some changes in 2020! :)

2020 has been a disappointment in so many ways that I'm not holding my breath. ☹️ 😄

[jhalterman]

In addition to the above, I'm thinking of leaving the event listener registration mutable (but threadsafe), rather than making them part of the builder. This might make it easier to integrate a policy with 3rd party monitoring, where the event listener hooks might be hard to add at the exact same time that the policy is built but are easy to add afterwards.

[reutsharabani]

Thank you for your work on this library :)