Excavator: Upgrades Baseline to the latest version #5169

svc-excavator-bot · 2020-12-22T00:04:55Z

excavator is a bot for automating changes across repositories.

Changes produced by the roomba/latest-baseline-oss check.

To enable or disable this check, please contact the maintainers of Excavator.

sudiksha27 · 2021-01-07T08:48:50Z

atlasdb-api/src/main/java/com/palantir/atlasdb/factory/TransactionManagerConsistencyResult.java

@@ -23,7 +23,9 @@
 * determine that it is safe (or, at least, not obviously unsafe) to service requests.
 */
 @Value.Immutable
+@SuppressWarnings("ClassInitializationDeadlock")
 public interface TransactionManagerConsistencyResult {


The immutable is used outside of package

sudiksha27 · 2021-01-07T08:50:36Z

...db-client/src/main/java/com/palantir/atlasdb/stream/StreamStorePersistenceConfiguration.java

@@ -23,10 +23,11 @@
 @JsonSerialize(as = ImmutableStreamStorePersistenceConfiguration.class)
 @JsonDeserialize(as = ImmutableStreamStorePersistenceConfiguration.class)
 @Value.Immutable
+@SuppressWarnings("ClassInitializationDeadlock")
 public interface StreamStorePersistenceConfiguration {


The immutable is used outside this package.

sudiksha27 · 2021-01-07T08:51:54Z

atlasdb-conjure/src/main/java/com/palantir/atlasdb/http/v2/ClientOptions.java

-            .failedUrlCooldown(Duration.ofMillis(1))
-            .maxNumRetries(5)
-            .clientQoS(ClientConfiguration.ClientQoS.DANGEROUS_DISABLE_SYMPATHETIC_CLIENT_QOS)
-            .build();



This constant is only used for testing.

I will assume that you verified this via a search on github? If so, great!

I verified there are. no usages on internal github.

sudiksha27 · 2021-01-07T09:00:55Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/Token.java

@@ -19,6 +19,8 @@
 import org.immutables.value.Value;

 @Value.Immutable
+@Value.Style(visibility = Value.Style.ImplementationVisibility.PACKAGE)
+@SuppressWarnings("ClassInitializationDeadlock")
 abstract class Token {
    @Nullable


Instead of breaking the interface contract by moving the constant, we are setting the impl visibility to PACKAGE (this is done to avoid initializing the immutable first) and suppressing the error.

To clarify, is it the case that:

setting the implementation visibility prevents the actual issue of initialisation deadlock?

the check is not smart enough to determine this, and so it requires the warning to be suppressed?

we cannot do this elsewhere because in other cases, the impl cannot be made package private? (In other words, why did we go with this solution here but not elsewhere, and vice versa?)

setting the implementation visibility only helps avoid external usages which could potentially cause a deadlock and within the package we have to be sensible while using the immutable.
The check is still right in this case to break, which is why we suppress it.
This cannot be done in the cases where the subclass has usages in other packages. In those cases we move out the constants to a separate util. To have a separate a util only for a constant seems a but janky which is why we have take the first approach if possible.

If we try to initialise ImmutableClass before Class, that will require us to initialise Class, but because of the static constant, that requires an initialisation of ImmutableClass and this deadlocks because there is a lock on ImmutableClass.

If you ensure that Class gets initialised first, then everything works fine, so by making access to ImmutableClass package private (to prevent external badness) and exposing a builder on the Class interface to be used instead of the ImmutableClass builder directly, we can avoid this issue

Package private may still be accessed externally. I considered special casing in the errorprone rule, but it would require additional validation that nothing accesses package-private generated immutables outside of the defining class, which we've seen more frequently than one would hope.

sudiksha27 · 2021-01-07T10:13:45Z

...f/src/main/java/com/palantir/atlasdb/performance/backend/KeyValueServiceInstrumentation.java

@@ -23,6 +23,7 @@
 import java.util.Set;
 import java.util.TreeMap;

+@SuppressWarnings("ClassInitializationDeadlock")
 public abstract class KeyValueServiceInstrumentation {


The CassandraKeyValueServiceInstrumentation is being used in tests in a different package and can not be made package private, would appreciate alternative ideas for fixing the ClassInitDeadlock issue here.

You can still make the generated code package private, and provide a builder in this class to avoid the issue outside of the package

This is only used in benchmarking, and not worth the effort to clean it up

Jolyon-S

🏁 00:10 | ⌛ 00:10

Thanks for doing this @sudiksha27. I think that there are two viable approaches for most of the fixes - deprecating and moving out those arbitrary constants (with a suppression of the warning in the mean time), and making package private and suppressing the warnings. However, I'm not too certain as to why we should use both approaches - are there times when one makes sense but not the other?

Jolyon-S · 2021-01-07T10:54:16Z

...db-client/src/main/java/com/palantir/atlasdb/stream/StreamStorePersistenceConfiguration.java

    StreamStorePersistenceConfiguration DEFAULT_CONFIG =
            ImmutableStreamStorePersistenceConfiguration.builder().build();
-


nit: unnecessary diff?

Jolyon-S · 2021-01-07T10:54:57Z

atlasdb-conjure/src/main/java/com/palantir/atlasdb/http/v2/ClientOptions.java

-            .failedUrlCooldown(Duration.ofMillis(1))
-            .maxNumRetries(5)
-            .clientQoS(ClientConfiguration.ClientQoS.DANGEROUS_DISABLE_SYMPATHETIC_CLIENT_QOS)
-            .build();



I will assume that you verified this via a search on github? If so, great!

Jolyon-S · 2021-01-07T10:57:32Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/Token.java

@@ -19,6 +19,8 @@
 import org.immutables.value.Value;

 @Value.Immutable
+@Value.Style(visibility = Value.Style.ImplementationVisibility.PACKAGE)
+@SuppressWarnings("ClassInitializationDeadlock")
 abstract class Token {
    @Nullable


To clarify, is it the case that:

setting the implementation visibility prevents the actual issue of initialisation deadlock?

the check is not smart enough to determine this, and so it requires the warning to be suppressed?

we cannot do this elsewhere because in other cases, the impl cannot be made package private? (In other words, why did we go with this solution here but not elsewhere, and vice versa?)

Jolyon-S · 2021-01-07T10:58:27Z

...in/java/com/palantir/atlasdb/performance/backend/PostgresKeyValueServiceInstrumentation.java

-public class PostgresKeyValueServiceInstrumentation extends KeyValueServiceInstrumentation {
+class PostgresKeyValueServiceInstrumentation extends KeyValueServiceInstrumentation {


Is this breaking?

Jolyon-S · 2021-01-07T10:59:03Z

lock-api/src/main/java/com/palantir/lock/LockServerOptions.java

+@Value.Style(visibility = Value.Style.ImplementationVisibility.PACKAGE)
+@SuppressWarnings("ClassInitializationDeadlock")


nit: order is not consistent with the order of annotations on some of the other classes

gmaretic

Can you make sure that you have the builders on the interfaces / abstract classes that have the constants and that the immutable builders are not used directly?

gmaretic · 2021-01-08T13:04:09Z

atlasdb-api/src/main/java/com/palantir/atlasdb/factory/TransactionManagerConsistencyResult.java

@@ -23,7 +23,9 @@
 * determine that it is safe (or, at least, not obviously unsafe) to service requests.
 */
 @Value.Immutable
+@SuppressWarnings("ClassInitializationDeadlock")


Suggested change

@SuppressWarnings("ClassInitializationDeadlock")

@SuppressWarnings("ClassInitializationDeadlock")

@Value.Style(visibility = Value.Style.ImplementationVisibility.PACKAGE)

And add a builder if necessary for outside use, the idea is not to expose the generated immutable

Curious why suppress an error-prone check for potential class initialization deadlock (see palantir/gradle-baseline#1598 that @carterkozak just added )? I'm assuming this is a fairly common pattern in many uses of immutables where there's either some constant defined as the empty object or similar. Do we need to consider pulling these types of constants out to a separate class (e.g. TransactionManagerConsistencyResults) or lazy initialization get of a memoized supplier?

gmaretic · 2021-01-08T13:04:54Z

...db-client/src/main/java/com/palantir/atlasdb/stream/StreamStorePersistenceConfiguration.java

@@ -23,7 +23,9 @@
 @JsonSerialize(as = ImmutableStreamStorePersistenceConfiguration.class)
 @JsonDeserialize(as = ImmutableStreamStorePersistenceConfiguration.class)
 @Value.Immutable
+@SuppressWarnings("ClassInitializationDeadlock")


Suggested change

@SuppressWarnings("ClassInitializationDeadlock")

@SuppressWarnings("ClassInitializationDeadlock")

@Value.Style(visibility = Value.Style.ImplementationVisibility.PACKAGE)

gmaretic · 2021-01-08T13:13:56Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/Token.java

@@ -19,6 +19,8 @@
 import org.immutables.value.Value;

 @Value.Immutable
+@Value.Style(visibility = Value.Style.ImplementationVisibility.PACKAGE)
+@SuppressWarnings("ClassInitializationDeadlock")
 abstract class Token {
    @Nullable


If we try to initialise ImmutableClass before Class, that will require us to initialise Class, but because of the static constant, that requires an initialisation of ImmutableClass and this deadlocks because there is a lock on ImmutableClass.

If you ensure that Class gets initialised first, then everything works fine, so by making access to ImmutableClass package private (to prevent external badness) and exposing a builder on the Class interface to be used instead of the ImmutableClass builder directly, we can avoid this issue

This reverts commit 0a5c839.

sudiksha27 · 2021-01-21T16:06:24Z

We came up with two options →

Parent class does not initialize subclass → Moving out those static immutable constants to a separate class. Still removing these constants from the interface would break the interface. For public interfaces, we can still keep the constant in the original place as well, mark it as deprecated and suppress the error. The we can remove the deprecated constants in later release.
Ensure immutable class is initialized first → Set impl visibility of the ImmutableClass to package private (to prevent external badness), expose a builder on the interface to be used instead of the ImmutableClass builder directly and suppress the error. This could also be a breaking change for currently public immutableClass. We also have to be sensible that the immutable class is not initialized first within the package.

Given both changes can be breaking, we're inclined towards solution 1 as it will completely remove the code that is vulnerable to ClassInitDeadlock.

gmaretic

LGTM

svc-autorelease · 2021-01-26T11:06:24Z

Released 0.289.0

carterkozak · 2021-01-26T17:52:52Z

Looks like this may have raced another PR which added a potential deadlock

svc-excavator-bot added the no changelog label Dec 22, 2020

svc-excavator-bot force-pushed the roomba/latest-baseline-oss branch from 012da33 to 4c08e41 Compare December 29, 2020 01:54

svc-excavator-bot force-pushed the roomba/latest-baseline-oss branch 3 times, most recently from 7b38285 to 1859042 Compare January 5, 2021 22:48

Excavator: Upgrades Baseline to the latest version

b60ffa5

svc-excavator-bot force-pushed the roomba/latest-baseline-oss branch from 6dab4d8 to b60ffa5 Compare January 6, 2021 13:23

sudiksha27 added 4 commits January 6, 2021 13:37

Fix ClassInitializationDeadlock - 1

ba6a9eb

ClassInitializationDeadlock fixes - 2

cfbafab

ClassInitializationDeadlock fixes - 3

86507d8

ClassInitializationDeadlock fixes - 4

8455973

sudiksha27 reviewed Jan 7, 2021

View reviewed changes

sudiksha27 added 2 commits January 7, 2021 09:23

Immutable annotations

98cf197

Fix build

da4a269

sudiksha27 reviewed Jan 7, 2021

View reviewed changes

Jolyon-S reviewed Jan 7, 2021

View reviewed changes

Address comments - 1

c695a34

sudiksha27 added the pr: needs revision label Jan 8, 2021

gmaretic suggested changes Jan 8, 2021

View reviewed changes

sudiksha27 added 5 commits January 11, 2021 14:58

Instrument batcher

0a5c839

Revert "Instrument batcher"

44902a3

This reverts commit 0a5c839.

Extract INITIAL Token

262661e

Extract out constants

2340d12

Fix build

346f1f9

gmaretic approved these changes Jan 26, 2021

View reviewed changes

sudiksha27 added autorelease and removed pr: needs revision labels Jan 26, 2021

gmaretic merged commit e75db01 into develop Jan 26, 2021

delete-merged-branch bot deleted the roomba/latest-baseline-oss branch January 26, 2021 11:06

sudiksha27 mentioned this pull request Jan 26, 2021

Fix ClassInitializationDeadlock #5226

Closed

pkoenig10 mentioned this pull request Jan 27, 2021

Suppress deprecation warnings in generated code #5229

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Excavator: Upgrades Baseline to the latest version #5169

Excavator: Upgrades Baseline to the latest version #5169

svc-excavator-bot commented Dec 22, 2020

sudiksha27 Jan 7, 2021

sudiksha27 Jan 7, 2021

sudiksha27 Jan 7, 2021

Jolyon-S Jan 7, 2021

sudiksha27 Jan 7, 2021

sudiksha27 Jan 7, 2021

Jolyon-S Jan 7, 2021

sudiksha27 Jan 7, 2021

gmaretic Jan 8, 2021

carterkozak Jan 8, 2021

sudiksha27 Jan 7, 2021

gmaretic Jan 8, 2021

gmaretic Jan 26, 2021

Jolyon-S left a comment •

edited

Loading

Jolyon-S Jan 7, 2021

Jolyon-S Jan 7, 2021

Jolyon-S Jan 7, 2021

Jolyon-S Jan 7, 2021

Jolyon-S Jan 7, 2021

gmaretic left a comment

gmaretic Jan 8, 2021

gmaretic Jan 8, 2021

schlosna Jan 8, 2021

gmaretic Jan 8, 2021

gmaretic Jan 8, 2021

sudiksha27 commented Jan 21, 2021

gmaretic left a comment

svc-autorelease commented Jan 26, 2021

carterkozak commented Jan 26, 2021

		StreamStorePersistenceConfiguration DEFAULT_CONFIG =
		ImmutableStreamStorePersistenceConfiguration.builder().build();

		public class PostgresKeyValueServiceInstrumentation extends KeyValueServiceInstrumentation {
		class PostgresKeyValueServiceInstrumentation extends KeyValueServiceInstrumentation {

		@Value.Style(visibility = Value.Style.ImplementationVisibility.PACKAGE)
		@SuppressWarnings("ClassInitializationDeadlock")

Excavator: Upgrades Baseline to the latest version #5169

Excavator: Upgrades Baseline to the latest version #5169

Conversation

svc-excavator-bot commented Dec 22, 2020

excavator is a bot for automating changes across repositories.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jolyon-S left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmaretic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudiksha27 commented Jan 21, 2021

gmaretic left a comment

Choose a reason for hiding this comment

svc-autorelease commented Jan 26, 2021

carterkozak commented Jan 26, 2021

Jolyon-S left a comment •

edited

Loading