Make Backoff class thread-safe by michaeljmarshall · Pull Request #13993 · apache/pulsar

michaeljmarshall · 2022-01-27T23:18:06Z

Motivation

In preparing #13951 (still a draft at this time), I noticed that the Backoff class is not thread safe, but it is accessed as if it were because we pass it to multiple threads and call .next() to get the next backoff delay.

Modifications

Move assertions for initial and max values to constructor and remove them from the RetryUtil class.
Remove the @Data annotation on the Backoff class. It allowed for unsafe access to internal state.
Add Javadocs to improve documentation of the class.
Remove unsafe initialization for the next variable. Here is my source showing that this initialization is unsafe: https://shipilev.net/blog/2014/safe-public-construction/.

Verifying this change

There is already test coverage for this class. The primary changes just add synchronized keywords to methods that get or update mutable state.

Does this pull request potentially affect one of the following parts:

It's possible that removing the @Data annotation on the class will break client applications since Backoff is a public class in the java client. I'll need guidance on how we should proceed here.

Documentation

doc

This PR contains Javadocs for relevant documentation. There is no need to document this change elsewhere.

mattisonchao · 2022-01-27T23:48:58Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/Backoff.java

        this.mandatoryStopMade = false;
    }

+    public synchronized boolean isMandatoryStopMade() {


This method looks like just getting the current value, should we synchronize it?

We either need to synchronize it or make the variable volatile. Since it is only used in one place (and is possibly used incorrectly), I lean towards making it synchronized, for now.

mattisonchao · 2022-01-27T23:49:32Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/Backoff.java

+
    @VisibleForTesting
-    long getFirstBackoffTimeInMillis() {
+    synchronized long getFirstBackoffTimeInMillis() {


same above. 👆

Same comment as above, except this is only used for a test.

lhotari · 2022-01-28T16:19:43Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/Backoff.java

+import lombok.Getter;

-// All variables are in TimeUnit millis by default
-@Data


I guess @Data should be replaced with @ToString @EqualsAndHashCode if the intention is to remove setters. @Value would be the correct solution if Backoff would be fully immutable.

https://projectlombok.org/features/Data is the docs for @Data

Great point. Do you think that I also need to make the mutable fields volatile then, since toString() and .equals won't be synchronized?

Great point. Do you think that I also need to make the mutable fields volatile then, since toString() and .equals won't be synchronized?

I think that it would be an overkill to use volatile in this case. In Java with 64 bit JVMs you cannot hit a "word tearing" issue where the observed field value would be corrupted. The worst that could happen in Java (on a 64 bit JVM) is a data race.

I added in the annotations.

Interestingly, it looks like spot bugs didn't like the change for the lack of synchronization introduced by the annotations:

Error: Medium: Inconsistent synchronization of org.apache.pulsar.client.impl.Backoff.next; locked 75% of time [org.apache.pulsar.client.impl.Backoff, org.apache.pulsar.client.impl.Backoff, org.apache.pulsar.client.impl.Backoff, org.apache.pulsar.client.impl.Backoff, org.apache.pulsar.client.impl.Backoff, org.apache.pulsar.client.impl.Backoff, org.apache.pulsar.client.impl.Backoff, org.apache.pulsar.client.impl.Backoff, org.apache.pulsar.client.impl.Backoff, org.apache.pulsar.client.impl.Backoff, org.apache.pulsar.client.impl.Backoff, org.apache.pulsar.client.impl.Backoff, org.apache.pulsar.client.impl.Backoff] Unsynchronized access at Backoff.java:[line 35]Unsynchronized access at Backoff.java:[line 35]Unsynchronized access at Backoff.java:[line 35]Synchronized access at Backoff.java:[line 96]Synchronized access at Backoff.java:[line 99]Synchronized access at Backoff.java:[line 97]Synchronized access at Backoff.java:[line 101]Synchronized access at Backoff.java:[line 101]Synchronized access at Backoff.java:[line 142]Synchronized access at Backoff.java:[line 133]Synchronized access at Backoff.java:[line 134]Synchronized access at Backoff.java:[line 134]Synchronized access at Backoff.java:[line 34] IS2_INCONSISTENT_SYNC

I don't see any use for equality checking of Backoff objects outside of reference equality, and calling toString on the class is going to log a lot of information. I am leaning towards just removing these annotations. If we really want toString, I can write one in the class.

@lhotari - are you able to take a look at this again?

This reverts commit bf50f7b.

…lass" This reverts commit 4331d86.

…ig"" This reverts commit d23db02.

github-actions · 2022-03-19T01:55:55Z

The pr had no activity for 30 days, mark with Stale label.

github-actions · 2022-05-28T02:11:52Z

The pr had no activity for 30 days, mark with Stale label.

tisonkun · 2023-03-28T09:37:36Z

Closing as stale...

We later duplicate BackOff in another module - #10744

Maybe we should move this class to pulsar-common and do the refactor if it's still relevant.

tisonkun · 2023-03-28T09:43:31Z

pass it to multiple threads

Another approach is we pass always the builder and initialize the instance per thread (call graph). It seems more reasonable.

Make Backoff class thread-safe

cfa653e

michaeljmarshall added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. area/client labels Jan 27, 2022

michaeljmarshall added this to the 2.10.0 milestone Jan 27, 2022

michaeljmarshall requested review from 315157973, BewareMyPower, eolivelli, jerrypeng, lhotari and merlimat January 27, 2022 23:18

michaeljmarshall self-assigned this Jan 27, 2022

mattisonchao previously approved these changes Jan 27, 2022

View reviewed changes

mattisonchao reviewed Jan 27, 2022

View reviewed changes

michaeljmarshall added 2 commits January 28, 2022 00:44

Improve error message for Backoff

ea11987

Fix test by using default config instead of mock config

bf50f7b

lhotari reviewed Jan 28, 2022

View reviewed changes

michaeljmarshall added 4 commits January 28, 2022 14:07

Add @tostring and @EqualsAndHashCode annotations to Backoff class

4331d86

Revert "Fix test by using default config instead of mock config"

d23db02

This reverts commit bf50f7b.

Revert "Add @tostring and @EqualsAndHashCode annotations to Backoff c…

fc017e9

…lass" This reverts commit 4331d86.

Revert "Revert "Fix test by using default config instead of mock conf…

fde3c47

…ig"" This reverts commit d23db02.

codelipenghui modified the milestones: 2.10.0, 2.11.0 Feb 8, 2022

congbobo184 dismissed mattisonchao’s stale review via fde3c47 February 16, 2022 02:58

github-actions bot added the lifecycle/stale label Mar 19, 2022

codelipenghui modified the milestones: 2.11.0, 2.12.0 Jul 26, 2022

tisonkun closed this Mar 28, 2023

Conversation

michaeljmarshall commented Jan 27, 2022

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhotari Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 19, 2022

Uh oh!

github-actions bot commented May 28, 2022

Uh oh!

tisonkun commented Mar 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tisonkun commented Mar 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lhotari Jan 28, 2022 •

edited

Loading

tisonkun commented Mar 28, 2023 •

edited

Loading

tisonkun commented Mar 28, 2023 •

edited

Loading