fix(jsii-pacmak): computeIfAbsent throws ConcurrentModificationException #1706

NetaNir · 2020-06-02T03:55:58Z

Java generated modules use cache to store the result of resolveClass which is called when ever jsiiStaticGet/jsiiGet is called. The cache is implemented as a HashMap, to fill the cache we use computeIfAbsent:

this.cache.computeIfAbsent(MODULE_TYPES.get(fqn), this::findClass);

code

When either of the get methods is called on a type which in itself includes a type, the first get (the root type get) will call get on the inner type. When computeIfAbsent is called from the root get, the compute method will trigger another call to computeIfAbsent to resolve the inner type, since all types share the same cache it means that the inner computeIfAbsent call will modify the cache while the root computeIfAbsent is not yet completed, resulting it a ConcurrentModificationException.

This effects all Java >= 9 applications as a change to the behavior of computeIfAbsent was introduced in Java 9. From the Javadocs:

Throws:
ConcurrentModificationException - if it is detected that the mapping function modified this map

The use of computeIfAbsent was introduced in #1605 and released in v1.4.1.

This PR replaces computeIfAbsent with explicitly checking if the value exists in the cache and if not adding it (essentially old style computeIfAbsent).

Test

Steps to verify fix:

Build and package a sample module (see repro) using a local version of jsii and jsii-pacmak.
Add the generated jar to a local mvn repo.
Added the package as a dependency to a Java11 application.
Verify the exception is no longer thrown.

Repro

A repro can be found in this repo

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

NetaNir · 2020-06-02T03:56:42Z

copy @iliapolo

aws-cdk-automation · 2020-06-02T04:05:42Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-Blkkw9bQFn8A
Commit ID: d82887b
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

eladb · 2020-06-02T04:15:25Z

🤩 wow

aws-cdk-automation · 2020-06-02T04:58:15Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-Blkkw9bQFn8A
Commit ID: 8fd896c
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-06-02T05:55:37Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-Blkkw9bQFn8A
Commit ID: 63d5d5d
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

eladb · 2020-06-02T06:04:27Z

@NetaNir What did we change that suddenly caused this? Can you please include a reference to the PR that introduced this break?

eladb · 2020-06-02T06:07:52Z

packages/jsii-pacmak/lib/targets/java.ts

-    );
+    this.code.line('String className = MODULE_TYPES.get(fqn);');
+    this.code.openBlock('if (!this.cache.containsKey(className))');
+    this.code.line('this.cache.put(className, this.findClass(className));');


Any chance this can cause an infinite recursion (which I assume is why computeIfAbsent throws this error)?

I think that there is a circuit breaker here, but I'll let @RomainMuller comment

I think this is going to be enough indeed. The problem was that further mutation to the map happened as part of this.findClass within the lambda expression passed to computeIfAbsent. This approach makes "atomic" mutations here (and this is safe because jsii apps can be expected to be single-threaded).

NetaNir · 2020-06-02T06:16:24Z

@NetaNir What did we change that suddenly caused this? Can you please include a reference to the PR that introduced this break?

@eladb updated description to include the PR which introduced the use of computeIfAbsent

eladb · 2020-06-02T07:14:01Z

@NetaNir What did we change that suddenly caused this? Can you please include a reference to the PR that introduced this break?

@eladb updated description to include the PR which introduced the use of computeIfAbsent

Thanks!

eladb

Is there a way to add a unit test for this?

rix0rrr · 2020-06-02T08:02:47Z

When computeIfAbsent is called from the root get, the compute method will trigger another call to computeIfAbsent to resolve the inner type,

We kinda knew that this would have to be the issue, but do we know HOW/WHY?

How in the hell does Class.forName(...) ultimately lead back to $Module.resolveClass()?

(I also originally expected $Module.resolveClass() to be in the stack trace twice, but of course that's not true. The ultimate complete stack trace would look like this:

(stack goes ⬇️ that way)
(...some stack frames...)
$Module.resolveClass
HashMap.computeIfAbsent      <----- 2️⃣ Error occurs here *AFTER* the frames below have unwound
$Module.findClass
Class.forName
(... what goes here...?)
$Module.resolveClass
HashMap.computeIfAbsent       <---- 1️⃣ This completes successfully first

But who can fill in the (... what goes here...) section for me?

RomainMuller · 2020-06-02T09:05:20Z

@rix0rrr to answer your (... what goes here...) section:

Class.forName will possibly load up a newly discovered class (let's call it Foo)
- Can be a base class/interface, some property's type, ...
Class initialization of Foo might involve jsii calls
- Getting values of const fields, for example
The result of those calls may be something that $Module.resolveClass is used for (e.g: class/interface instance, struct, enum)
- At this point, we'll get back to computeIfAbsent

RomainMuller · 2020-06-02T09:09:35Z

I'm trying to come up with a test that blows up before the fix is introduced.

rix0rrr · 2020-06-02T09:10:06Z

Getting values of const fields, for example

Got it. So that should be easy enough to write a test for then. So it's basically a matter of:

class SomeClass {
  public static const OTHER: OtherClass = new OtherClass();
}

?

rix0rrr · 2020-06-02T09:10:44Z

Although of course that won't break if OtherClass is referenced in any other capacity...

RomainMuller · 2020-06-02T09:53:59Z

Getting values of const fields, for example

Got it. So that should be easy enough to write a test for then. So it's basically a matter of:

Yeah that's what I thought but it appears the patch to trigger this situation depends on a bunch of other factors I'm trying to identify (possibly the properties being typed "any" or something.

RomainMuller · 2020-06-02T09:56:33Z

Oh I missed a critical element of the repro: it only happens with Java >= 9. It seems related to a change in class loading behavior between 8 and 9+.

iliapolo · 2020-06-02T10:18:23Z

@RomainMuller Its not a change in class loading, its a change in the implementation of computeIfAbsent, which is Java >= 9 throws the ConcurrentModificationException when it detects a dirty mutation.

https://stackoverflow.com/questions/54824656/since-java-9-hashmap-computeifabsent-throws-concurrentmodificationexception-on

Regarding the repro, did you look at @NetaNir's repo?

It wasn't that straight-forward to reproduce, but I do think the repro can probably be simplified.

Also, since the recursion stems from Class.forName, it can only happen when a class has static initializers that eventually call jsiiGet - right? Since Class.forName only loads the class, and doesn't instantiate it.

RomainMuller · 2020-06-02T10:19:35Z

So the flow is:

Resolving some class involves loading another
Loading the other class involves resolving yet another

An easy way to trigger is to have a const property causing loading of an enum (it'll then try to resolve the enum when loading up it's values)

Added a test to this effect. Interestingly, it works in Java 8, fails in Java 9. I suspect Java 9 resolves classes more eagerly than 8.

iliapolo · 2020-06-02T10:22:20Z

@RomainMuller

Interestingly, it works in Java 8, fails in Java 9. I suspect Java 9 resolves classes more eagerly than 8.

https://stackoverflow.com/questions/54824656/since-java-9-hashmap-computeifabsent-throws-concurrentmodificationexception-on

aws-cdk-automation · 2020-06-02T10:28:10Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-Blkkw9bQFn8A
Commit ID: ac7b8c7
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

RomainMuller · 2020-06-02T10:33:46Z

@RomainMuller

Interestingly, it works in Java 8, fails in Java 9. I suspect Java 9 resolves classes more eagerly than 8.

https://stackoverflow.com/questions/54824656/since-java-9-hashmap-computeifabsent-throws-concurrentmodificationexception-on

Ohhhhh

aws-cdk-automation · 2020-06-02T11:03:11Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-Blkkw9bQFn8A
Commit ID: 38cde99
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-06-02T11:27:57Z

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

mergify · 2020-06-02T11:28:34Z

Merging (with squash)...

mergify · 2020-06-02T12:02:55Z

Command refresh: success

Neta Nir added 2 commits June 1, 2020 19:46

fix(jsii-pacmak): computeIfAbsent throws ConcurrentModificationException

939b901

remove console.log

d82887b

NetaNir requested review from RomainMuller, rix0rrr, MrArnoldPalmer and iliapolo June 2, 2020 03:56

mergify bot added the contribution/core This is a PR that came from AWS. label Jun 2, 2020

NetaNir self-assigned this Jun 2, 2020

Merge branch 'master' into neta/nested-get-fix

8fd896c

update test expectation files

63d5d5d

eladb reviewed Jun 2, 2020

View reviewed changes

NetaNir added the p0 label Jun 2, 2020

eladb reviewed Jun 2, 2020

View reviewed changes

add unit test (java 9+)

ac7b8c7

RomainMuller approved these changes Jun 2, 2020

View reviewed changes

fix test expectations

38cde99

RomainMuller removed request for rix0rrr, iliapolo and MrArnoldPalmer June 2, 2020 11:27

mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Jun 2, 2020

mergify bot merged commit fa60b7f into master Jun 2, 2020

mergify bot deleted the neta/nested-get-fix branch June 2, 2020 11:28

mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Jun 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(jsii-pacmak): computeIfAbsent throws ConcurrentModificationException #1706

fix(jsii-pacmak): computeIfAbsent throws ConcurrentModificationException #1706

NetaNir commented Jun 2, 2020 •

edited

Loading

NetaNir commented Jun 2, 2020

aws-cdk-automation commented Jun 2, 2020

eladb commented Jun 2, 2020

aws-cdk-automation commented Jun 2, 2020

aws-cdk-automation commented Jun 2, 2020

eladb commented Jun 2, 2020

eladb Jun 2, 2020

NetaNir Jun 2, 2020 •

edited

Loading

RomainMuller Jun 2, 2020

NetaNir commented Jun 2, 2020

eladb commented Jun 2, 2020

eladb left a comment

rix0rrr commented Jun 2, 2020 •

edited

Loading

RomainMuller commented Jun 2, 2020

RomainMuller commented Jun 2, 2020

rix0rrr commented Jun 2, 2020

rix0rrr commented Jun 2, 2020

RomainMuller commented Jun 2, 2020

RomainMuller commented Jun 2, 2020

iliapolo commented Jun 2, 2020 •

edited

Loading

RomainMuller commented Jun 2, 2020

iliapolo commented Jun 2, 2020

aws-cdk-automation commented Jun 2, 2020

RomainMuller commented Jun 2, 2020

aws-cdk-automation commented Jun 2, 2020

mergify bot commented Jun 2, 2020

mergify bot commented Jun 2, 2020

mergify bot commented Jun 2, 2020

fix(jsii-pacmak): computeIfAbsent throws ConcurrentModificationException #1706

fix(jsii-pacmak): computeIfAbsent throws ConcurrentModificationException #1706

Conversation

NetaNir commented Jun 2, 2020 • edited Loading

Test

Repro

NetaNir commented Jun 2, 2020

aws-cdk-automation commented Jun 2, 2020

AWS CodeBuild CI Report

eladb commented Jun 2, 2020

aws-cdk-automation commented Jun 2, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 2, 2020

AWS CodeBuild CI Report

eladb commented Jun 2, 2020

eladb Jun 2, 2020

Choose a reason for hiding this comment

NetaNir Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

RomainMuller Jun 2, 2020

Choose a reason for hiding this comment

NetaNir commented Jun 2, 2020

eladb commented Jun 2, 2020

eladb left a comment

Choose a reason for hiding this comment

rix0rrr commented Jun 2, 2020 • edited Loading

RomainMuller commented Jun 2, 2020

RomainMuller commented Jun 2, 2020

rix0rrr commented Jun 2, 2020

rix0rrr commented Jun 2, 2020

RomainMuller commented Jun 2, 2020

RomainMuller commented Jun 2, 2020

iliapolo commented Jun 2, 2020 • edited Loading

RomainMuller commented Jun 2, 2020

iliapolo commented Jun 2, 2020

aws-cdk-automation commented Jun 2, 2020

AWS CodeBuild CI Report

RomainMuller commented Jun 2, 2020

aws-cdk-automation commented Jun 2, 2020

AWS CodeBuild CI Report

mergify bot commented Jun 2, 2020

mergify bot commented Jun 2, 2020

mergify bot commented Jun 2, 2020

NetaNir commented Jun 2, 2020 •

edited

Loading

NetaNir Jun 2, 2020 •

edited

Loading

rix0rrr commented Jun 2, 2020 •

edited

Loading

iliapolo commented Jun 2, 2020 •

edited

Loading