RFR: 8263995: Incorrect double-checked locking in Types.arraySuperType()

[Aleksey Shipilev-5]

In Types.arraySuperType(), SonarCloud reports:
 Remove this dangerous instance of double-checked locking.

Indeed, the `arraySuperType` is not `volatile`, while `IntersectionClassType` has non-`final` fields (both in itself and in superclasses). This is an incorrect DCL.

-------------

Commit messages:
 - 8263995: Incorrect double-checked locking in Types.arraySuperType()

Changes: https://git.openjdk.java.net/jdk/pull/3129/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3129&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8263995
  Stats: 8 lines in 1 file changed: 3 ins; 0 del; 5 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3129.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3129/head:pull/3129

PR: https://git.openjdk.java.net/jdk/pull/3129

[Maurizio Cimadamore-2]

On Mon, 22 Mar 2021 18:19:39 GMT, Aleksey Shipilev <[hidden email]> wrote:

> In Types.arraySuperType(), SonarCloud reports:
>  Remove this dangerous instance of double-checked locking.
>
> Indeed, the `arraySuperType` is not `volatile`, while `IntersectionClassType` has non-`final` fields (both in itself and in superclasses). This is an incorrect DCL.

I'm a bit surprised to see use of volatile/synchronized here as I'm pretty sure that javac is *not* thread safe.
It seems like the synchronized block has been there from a long time (e.g. it was there since the dawn of OpenJDK :-)). I wonder if a better fix would be simply to drop synchronization and volatileness.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3129

[Aleksey Shipilev-5]

> In Types.arraySuperType(), SonarCloud reports:
>  Remove this dangerous instance of double-checked locking.
>
> Indeed, the `arraySuperType` is not `volatile`, while `IntersectionClassType` has non-`final` fields (both in itself and in superclasses). This is an incorrect DCL.

Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision:

  Drop synchronization and volatileness

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3129/files
  - new: https://git.openjdk.java.net/jdk/pull/3129/files/e8092ca6..aeee7536

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3129&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3129&range=00-01

  Stats: 13 lines in 1 file changed: 0 ins; 7 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3129.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3129/head:pull/3129

PR: https://git.openjdk.java.net/jdk/pull/3129

[Aleksey Shipilev-5]

On Mon, 22 Mar 2021 18:31:01 GMT, Maurizio Cimadamore <[hidden email]> wrote:

>> Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Drop synchronization and volatileness
>
> I'm a bit surprised to see use of volatile/synchronized here as I'm pretty sure that javac is *not* thread safe.
> It seems like the synchronized block has been there from a long time (e.g. it was there since the dawn of OpenJDK :-)). I wonder if a better fix would be simply to drop synchronization and volatileness.

> I'm a bit surprised to see use of volatile/synchronized here as I'm pretty sure that javac is _not_ thread safe.
> It seems like the synchronized block has been there from a long time (e.g. it was there since the dawn of OpenJDK :-)). I wonder if a better fix would be simply to drop synchronization and volatileness.

Yeah, I just did the most obvious semantics-preserving thing in this first patch. See new commit that drops `synchronized` instead as to not confuse both humans and tools about this. Still passes `langtools:tier1`.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3129

[Jan Lahoda-3]

On Mon, 22 Mar 2021 19:06:00 GMT, Aleksey Shipilev <[hidden email]> wrote:

>> In Types.arraySuperType(), SonarCloud reports:
>>  Remove this dangerous instance of double-checked locking.
>>
>> Indeed, the `arraySuperType` is not `volatile`, while `IntersectionClassType` has non-`final` fields (both in itself and in superclasses). This is an incorrect DCL.
>
> Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision:
>
>   Drop synchronization and volatileness

Looks good to me!

-------------

Marked as reviewed by jlahoda (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3129

[Maurizio Cimadamore-2]

On Mon, 22 Mar 2021 19:06:00 GMT, Aleksey Shipilev <[hidden email]> wrote:

>> In Types.arraySuperType(), SonarCloud reports:
>>  Remove this dangerous instance of double-checked locking.
>>
>> Indeed, the `arraySuperType` is not `volatile`, while `IntersectionClassType` has non-`final` fields (both in itself and in superclasses). This is an incorrect DCL.
>
> Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision:
>
>   Drop synchronization and volatileness

Looks good

-------------

Marked as reviewed by mcimadamore (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3129

[Aleksey Shipilev-5]

On Mon, 22 Mar 2021 18:19:39 GMT, Aleksey Shipilev <[hidden email]> wrote:

> In Types.arraySuperType(), SonarCloud reports:
>  Remove this dangerous instance of double-checked locking.
>
> Indeed, the `arraySuperType` is not `volatile`, while `IntersectionClassType` has non-`final` fields (both in itself and in superclasses). This is an incorrect DCL.

This pull request has now been integrated.

Changeset: c087f3ed
Author:    Aleksey Shipilev <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/c087f3ed
Stats:     8 lines in 1 file changed: 0 ins; 4 del; 4 mod

8263995: Incorrect double-checked locking in Types.arraySuperType()

Reviewed-by: mcimadamore, jlahoda

-------------

PR: https://git.openjdk.java.net/jdk/pull/3129