Skip to content

Commit

Permalink
Make lock objects transient.
Browse files Browse the repository at this point in the history 
This let us move away from the current hack (which was never in a Guava _release_) of using a lock of `new Integer(1)` (because we needed something `Serializable` while we figured this all out).

RELNOTES=n/a
PiperOrigin-RevId: 659714359
  • Loading branch information
@cpovirk
cpovirk authored and Google Java Core Libraries committed Aug 5, 2024
1 parent 641b4c5 commit 5f0e886
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 8 deletions.
26 changes: 22 additions & 4 deletions android/guava/src/com/google/common/base/Suppliers.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
import com.google.common.annotations.GwtIncompatible;
import com.google.common.annotations.J2ktIncompatible;
import com.google.common.annotations.VisibleForTesting;
import java.io.IOException;
import java.io.ObjectInputStream;
import java.io.Serializable;
import java.time.Duration;
import java.util.concurrent.TimeUnit;
Expand Down Expand Up @@ -120,8 +122,7 @@ public String toString() {

@VisibleForTesting
static class MemoizingSupplier<T extends @Nullable Object> implements Supplier<T>, Serializable {
private final Object lock =
new Integer(1); // something serializable
private transient Object lock = new Object();

final Supplier<T> delegate;
transient volatile boolean initialized;
Expand All @@ -135,6 +136,8 @@ static class MemoizingSupplier<T extends @Nullable Object> implements Supplier<T

@Override
@ParametricNullness
// We set the field only once (during construction or deserialization).
@SuppressWarnings("SynchronizeOnNonFinalField")
public T get() {
// A 2-field variant of Double Checked Locking.
if (!initialized) {
Expand All @@ -158,6 +161,13 @@ public String toString() {
+ ")";
}

@GwtIncompatible // serialization
@J2ktIncompatible // serialization
private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException {
in.defaultReadObject();
lock = new Object();
}

private static final long serialVersionUID = 0;
}

Expand Down Expand Up @@ -275,8 +285,7 @@ public String toString() {
@SuppressWarnings("GoodTime") // lots of violations
static class ExpiringMemoizingSupplier<T extends @Nullable Object>
implements Supplier<T>, Serializable {
private final Object lock =
new Integer(1); // something serializable
private transient Object lock = new Object();

final Supplier<T> delegate;
final long durationNanos;
Expand All @@ -291,6 +300,8 @@ static class ExpiringMemoizingSupplier<T extends @Nullable Object>

@Override
@ParametricNullness
// We set the field only once (during construction or deserialization).
@SuppressWarnings("SynchronizeOnNonFinalField")
public T get() {
// Another variant of Double Checked Locking.
//
Expand Down Expand Up @@ -324,6 +335,13 @@ public String toString() {
return "Suppliers.memoizeWithExpiration(" + delegate + ", " + durationNanos + ", NANOS)";
}

@GwtIncompatible // serialization
@J2ktIncompatible // serialization
private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException {
in.defaultReadObject();
lock = new Object();
}

private static final long serialVersionUID = 0;
}

Expand Down
26 changes: 22 additions & 4 deletions guava/src/com/google/common/base/Suppliers.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
import com.google.common.annotations.GwtIncompatible;
import com.google.common.annotations.J2ktIncompatible;
import com.google.common.annotations.VisibleForTesting;
import java.io.IOException;
import java.io.ObjectInputStream;
import java.io.Serializable;
import java.time.Duration;
import java.util.concurrent.TimeUnit;
Expand Down Expand Up @@ -120,8 +122,7 @@ public String toString() {

@VisibleForTesting
static class MemoizingSupplier<T extends @Nullable Object> implements Supplier<T>, Serializable {
private final Object lock =
new Integer(1); // something serializable
private transient Object lock = new Object();

final Supplier<T> delegate;
transient volatile boolean initialized;
Expand All @@ -135,6 +136,8 @@ static class MemoizingSupplier<T extends @Nullable Object> implements Supplier<T

@Override
@ParametricNullness
// We set the field only once (during construction or deserialization).
@SuppressWarnings("SynchronizeOnNonFinalField")
public T get() {
// A 2-field variant of Double Checked Locking.
if (!initialized) {
Expand All @@ -158,6 +161,13 @@ public String toString() {
+ ")";
}

@GwtIncompatible // serialization
@J2ktIncompatible // serialization
private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException {
in.defaultReadObject();
lock = new Object();
}

private static final long serialVersionUID = 0;
}

Expand Down Expand Up @@ -275,8 +285,7 @@ public String toString() {
@SuppressWarnings("GoodTime") // lots of violations
static class ExpiringMemoizingSupplier<T extends @Nullable Object>
implements Supplier<T>, Serializable {
private final Object lock =
new Integer(1); // something serializable
private transient Object lock = new Object();

final Supplier<T> delegate;
final long durationNanos;
Expand All @@ -291,6 +300,8 @@ static class ExpiringMemoizingSupplier<T extends @Nullable Object>

@Override
@ParametricNullness
// We set the field only once (during construction or deserialization).
@SuppressWarnings("SynchronizeOnNonFinalField")
public T get() {
// Another variant of Double Checked Locking.
//
Expand Down Expand Up @@ -324,6 +335,13 @@ public String toString() {
return "Suppliers.memoizeWithExpiration(" + delegate + ", " + durationNanos + ", NANOS)";
}

@GwtIncompatible // serialization
@J2ktIncompatible // serialization
private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException {
in.defaultReadObject();
lock = new Object();
}

private static final long serialVersionUID = 0;
}

Expand Down

2 comments on commit 5f0e886

@ben-manes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Object[0] as your lock object should mean you don’t have to customize the serialization since it’s handled specifically. It’s a leaner and safer trick.

@cpovirk
Copy link
Member Author

@cpovirk cpovirk commented on 5f0e886 Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Object[0] as your lock object should mean you don’t have to customize the serialization since it’s handled specifically. It’s a leaner and safer trick.

Aha, thanks, @ben-manes! As it happens, this commit is also trying to accommodate a couple pieces of Google-internal weirdness, so if it sticks, I am going to leave it alone, especially given the chance that we someday further rework it to temporarily address thread pinning for testing purposes after all. However, I also have another commit on the way to address some tests that need Serializable monitors, so I will be reworking that to use Object[0] instead of the custom SerializableMonitor class that I'd been planning to introduce.

Please sign in to comment.