You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/01/27 23:18:22 UTC
[GitHub] [pulsar] michaeljmarshall opened a new pull request #13993: Make Backoff class thread-safe
michaeljmarshall opened a new pull request #13993:
URL: https://github.com/apache/pulsar/pull/13993
### Motivation
In preparing https://github.com/apache/pulsar/pull/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
- [x] `doc`
This PR contains Javadocs for relevant documentation. There is no need to document this change elsewhere.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #13993: Make Backoff class thread-safe
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #13993:
URL: https://github.com/apache/pulsar/pull/13993#discussion_r794875265
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/Backoff.java
##########
@@ -22,29 +22,59 @@
import java.time.Clock;
import java.util.Random;
import java.util.concurrent.TimeUnit;
-import lombok.Data;
+import lombok.Getter;
-// All variables are in TimeUnit millis by default
-@Data
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a change in pull request #13993: Make Backoff class thread-safe
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #13993:
URL: https://github.com/apache/pulsar/pull/13993#discussion_r794650360
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/Backoff.java
##########
@@ -22,29 +22,59 @@
import java.time.Clock;
import java.util.Random;
import java.util.concurrent.TimeUnit;
-import lombok.Data;
+import lombok.Getter;
-// All variables are in TimeUnit millis by default
-@Data
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a change in pull request #13993: Make Backoff class thread-safe
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #13993:
URL: https://github.com/apache/pulsar/pull/13993#discussion_r794652099
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/Backoff.java
##########
@@ -22,29 +22,59 @@
import java.time.Clock;
import java.util.Random;
import java.util.concurrent.TimeUnit;
-import lombok.Data;
+import lombok.Getter;
-// All variables are in TimeUnit millis by default
-@Data
Review comment:
https://projectlombok.org/features/Data is the docs for `@Data`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #13993: Make Backoff class thread-safe
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #13993:
URL: https://github.com/apache/pulsar/pull/13993#discussion_r794199365
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/Backoff.java
##########
@@ -84,19 +121,30 @@ public long next() {
return Math.max(initial, current);
}
- public void reduceToHalf() {
+ /**
+ * Reduce the next backoff to half of its current value. If the result is less than {@link #initial}, set
+ * {@link #next} to {@link #initial}.
+ */
+ public synchronized void reduceToHalf() {
if (next > initial) {
this.next = Math.max(this.next / 2, this.initial);
}
}
- public void reset() {
+ /**
+ * Reset this {@link Backoff} instance to its initial state.
+ */
+ public synchronized void reset() {
this.next = this.initial;
this.mandatoryStopMade = false;
}
+ public synchronized boolean isMandatoryStopMade() {
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a change in pull request #13993: Make Backoff class thread-safe
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #13993:
URL: https://github.com/apache/pulsar/pull/13993#discussion_r794682764
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/Backoff.java
##########
@@ -22,29 +22,59 @@
import java.time.Clock;
import java.util.Random;
import java.util.concurrent.TimeUnit;
-import lombok.Data;
+import lombok.Getter;
-// All variables are in TimeUnit millis by default
-@Data
Review comment:
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #13993: Make Backoff class thread-safe
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #13993:
URL: https://github.com/apache/pulsar/pull/13993#discussion_r794865472
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/Backoff.java
##########
@@ -22,29 +22,59 @@
import java.time.Clock;
import java.util.Random;
import java.util.concurrent.TimeUnit;
-import lombok.Data;
+import lombok.Getter;
-// All variables are in TimeUnit millis by default
-@Data
Review comment:
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]Synchro
nized 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #13993: Make Backoff class thread-safe
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #13993:
URL: https://github.com/apache/pulsar/pull/13993#discussion_r794199468
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/Backoff.java
##########
@@ -84,19 +121,30 @@ public long next() {
return Math.max(initial, current);
}
- public void reduceToHalf() {
+ /**
+ * Reduce the next backoff to half of its current value. If the result is less than {@link #initial}, set
+ * {@link #next} to {@link #initial}.
+ */
+ public synchronized void reduceToHalf() {
if (next > initial) {
this.next = Math.max(this.next / 2, this.initial);
}
}
- public void reset() {
+ /**
+ * Reset this {@link Backoff} instance to its initial state.
+ */
+ public synchronized void reset() {
this.next = this.initial;
this.mandatoryStopMade = false;
}
+ public synchronized boolean isMandatoryStopMade() {
+ return mandatoryStopMade;
+ }
+
@VisibleForTesting
- long getFirstBackoffTimeInMillis() {
+ synchronized long getFirstBackoffTimeInMillis() {
Review comment:
Same comment as above, except this is only used for a test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #13993: Make Backoff class thread-safe
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #13993:
URL: https://github.com/apache/pulsar/pull/13993#discussion_r794654722
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/Backoff.java
##########
@@ -22,29 +22,59 @@
import java.time.Clock;
import java.util.Random;
import java.util.concurrent.TimeUnit;
-import lombok.Data;
+import lombok.Getter;
-// All variables are in TimeUnit millis by default
-@Data
Review comment:
Great point. Do you think that I also need to make the mutable fields `volatile` then, since `toString()` and `.equals` won't be synchronized?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a change in pull request #13993: Make Backoff class thread-safe
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #13993:
URL: https://github.com/apache/pulsar/pull/13993#discussion_r794650360
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/Backoff.java
##########
@@ -22,29 +22,59 @@
import java.time.Clock;
import java.util.Random;
import java.util.concurrent.TimeUnit;
-import lombok.Data;
+import lombok.Getter;
-// All variables are in TimeUnit millis by default
-@Data
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #13993: Make Backoff class thread-safe
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13993:
URL: https://github.com/apache/pulsar/pull/13993#issuecomment-1072916906
The pr had no activity for 30 days, mark with Stale label.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #13993: Make Backoff class thread-safe
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #13993:
URL: https://github.com/apache/pulsar/pull/13993#discussion_r794861678
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/Backoff.java
##########
@@ -22,29 +22,59 @@
import java.time.Clock;
import java.util.Random;
import java.util.concurrent.TimeUnit;
-import lombok.Data;
+import lombok.Getter;
-// All variables are in TimeUnit millis by default
-@Data
Review comment:
I added in the annotations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on a change in pull request #13993: Make Backoff class thread-safe
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #13993:
URL: https://github.com/apache/pulsar/pull/13993#discussion_r794094031
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/Backoff.java
##########
@@ -84,19 +121,30 @@ public long next() {
return Math.max(initial, current);
}
- public void reduceToHalf() {
+ /**
+ * Reduce the next backoff to half of its current value. If the result is less than {@link #initial}, set
+ * {@link #next} to {@link #initial}.
+ */
+ public synchronized void reduceToHalf() {
if (next > initial) {
this.next = Math.max(this.next / 2, this.initial);
}
}
- public void reset() {
+ /**
+ * Reset this {@link Backoff} instance to its initial state.
+ */
+ public synchronized void reset() {
this.next = this.initial;
this.mandatoryStopMade = false;
}
+ public synchronized boolean isMandatoryStopMade() {
+ return mandatoryStopMade;
+ }
+
@VisibleForTesting
- long getFirstBackoffTimeInMillis() {
+ synchronized long getFirstBackoffTimeInMillis() {
Review comment:
same above. 👆
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/Backoff.java
##########
@@ -84,19 +121,30 @@ public long next() {
return Math.max(initial, current);
}
- public void reduceToHalf() {
+ /**
+ * Reduce the next backoff to half of its current value. If the result is less than {@link #initial}, set
+ * {@link #next} to {@link #initial}.
+ */
+ public synchronized void reduceToHalf() {
if (next > initial) {
this.next = Math.max(this.next / 2, this.initial);
}
}
- public void reset() {
+ /**
+ * Reset this {@link Backoff} instance to its initial state.
+ */
+ public synchronized void reset() {
this.next = this.initial;
this.mandatoryStopMade = false;
}
+ public synchronized boolean isMandatoryStopMade() {
Review comment:
This method looks like just getting the current value, should we synchronize it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org