fix(jsii-pacmak): computeIfAbsent throws ConcurrentModificationExcept…

…ion (#1706) --- 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`: ```Java this.cache.computeIfAbsent(MODULE_TYPES.get(fqn), this::findClass); ``` > [code](https://github.com/aws/jsii/blob/master/packages/jsii-pacmak/lib/targets/java.ts#L2533) 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](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/HashMap.html#computeIfAbsent(K,java.util.function.Function)): ``` 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: 1. Build and package a sample module (see repro) using a local version of `jsii` and `jsii-pacmak`. 2. Add the generated jar to a local `mvn` repo. 3. Added the package as a dependency to a Java11 application. 4. Verify the exception is no longer thrown. ### Repro A repro can be found in this [repo](https://github.com/NetaNir/jsii-nested-get-repro) By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license]. [Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
aws · Jun 2, 2020 · fa60b7f · fa60b7f
1 parent b2aa424
commit fa60b7f
Show file tree

Hide file tree

Showing 26 changed files with 867 additions and 21 deletions.
diff --git a/packages/@jsii/java-runtime-test/.gitignore b/packages/@jsii/java-runtime-test/.gitignore
@@ -1,3 +1,4 @@
+.m2
 .vscode
 !*.js
 maven-repo

diff --git a/packages/@jsii/java-runtime-test/generate.sh b/packages/@jsii/java-runtime-test/generate.sh
@@ -5,6 +5,9 @@ set -euo pipefail
 staging="maven-repo"
 rm -fr ${staging} && mkdir -p ${staging}
 
+# Remove local artifacts from local maven repository
+rm -fr project/.m2/software/amazon/jsii
+
 # generate user.xml & pom.xml
 node ./user.xml.t.js > ./project/user.xml
 node ./pom.xml.t.js > ./project/pom.xml

diff --git a/.../java-runtime-test/project/src/test/java/software/amazon/jsii/testing/ComplianceTest.java b/.../java-runtime-test/project/src/test/java/software/amazon/jsii/testing/ComplianceTest.java
@@ -17,6 +17,7 @@
 import software.amazon.jsii.tests.calculator.lib.Number;
 import software.amazon.jsii.tests.calculator.lib.StructWithOnlyOptionals;
 import software.amazon.jsii.tests.calculator.lib.Value;
+import software.amazon.jsii.tests.calculator.submodule.child.OuterClass;
 
 import java.io.IOException;
 import java.lang.reflect.Constructor;
@@ -1758,4 +1759,10 @@ public void collectionOfInterfaces_MapOfInterfaces() {
             assertTrue(obj instanceof IBell, () -> obj + " is an instance of " + IBell.class.getCanonicalName());
         }
     }
+
+    @Test
+    public void classesCanSelfReferenceDuringClassInitialization() {
+        final OuterClass outerClass = new OuterClass();
+        assertNotNull(outerClass.getInnerClass());
+    }
 }
diff --git a/packages/@jsii/java-runtime-test/user.xml.t.js b/packages/@jsii/java-runtime-test/user.xml.t.js
@@ -1,4 +1,5 @@
 const jsiiJavaRuntime = require('@jsii/java-runtime');
+const path = require('path');
 
 process.stdout.write(`<?xml version="1.0" encoding="UTF-8"?>
 <settings xmlns="http://maven.apache.org/POM/4.0.0"
@@ -7,17 +8,21 @@ process.stdout.write(`<?xml version="1.0" encoding="UTF-8"?>
 
     <!-- Generated by ${__filename} at ${new Date().toISOString()} -->
 
+    <localRepository>${path.resolve(__dirname, 'project', '.m2', 'repository')}</localRepository>
+
     <profiles>
         <profile>
             <id>local</id>
             <repositories>
                 <repository>
                     <id>jsii-runtime</id>
+                    <name>Locally built jsii runtime for Java</name>
                     <url>file://${jsiiJavaRuntime.repository}</url>
                 </repository>
                 <repository>
                     <id>jsii-calc</id>
-                    <url>file://${process.cwd()}/maven-repo</url>
+                    <name>Locally built artifacts for jsii-calc and dependencies</name>
+                    <url>file://${__dirname}/maven-repo/java</url>
                 </repository>
             </repositories>
         </profile>

diff --git a/packages/jsii-calc/lib/submodule/child/index.ts b/packages/jsii-calc/lib/submodule/child/index.ts
@@ -16,3 +16,26 @@ export enum Awesomeness {
   /** It was awesome! */
   AWESOME
 }
+
+/**
+ * Checks that classes can self-reference during initialization.
+ * @see: https://github.com/aws/jsii/pull/1706
+ */
+export class OuterClass {
+  public readonly innerClass: InnerClass;
+
+  public constructor() {
+    this.innerClass = new InnerClass();
+  }
+}
+export enum SomeEnum {
+  SOME = 'SOME'
+}
+export interface SomeStruct {
+  readonly prop: SomeEnum;
+}
+export class InnerClass {
+  public static readonly staticProp: SomeStruct = { prop: SomeEnum.SOME };
+
+  public constructor() { }
+}
diff --git a/packages/jsii-calc/test/assembly.jsii b/packages/jsii-calc/test/assembly.jsii
@@ -12885,6 +12885,134 @@
       "name": "Goodness",
       "namespace": "submodule.child"
     },
+    "jsii-calc.submodule.child.InnerClass": {
+      "assembly": "jsii-calc",
+      "docs": {
+        "stability": "experimental"
+      },
+      "fqn": "jsii-calc.submodule.child.InnerClass",
+      "initializer": {
+        "docs": {
+          "stability": "experimental"
+        }
+      },
+      "kind": "class",
+      "locationInModule": {
+        "filename": "lib/submodule/child/index.ts",
+        "line": 37
+      },
+      "name": "InnerClass",
+      "namespace": "submodule.child",
+      "properties": [
+        {
+          "const": true,
+          "docs": {
+            "stability": "experimental"
+          },
+          "immutable": true,
+          "locationInModule": {
+            "filename": "lib/submodule/child/index.ts",
+            "line": 38
+          },
+          "name": "staticProp",
+          "static": true,
+          "type": {
+            "fqn": "jsii-calc.submodule.child.SomeStruct"
+          }
+        }
+      ]
+    },
+    "jsii-calc.submodule.child.OuterClass": {
+      "assembly": "jsii-calc",
+      "docs": {
+        "see": ": https://github.com/aws/jsii/pull/1706",
+        "stability": "experimental",
+        "summary": "Checks that classes can self-reference during initialization."
+      },
+      "fqn": "jsii-calc.submodule.child.OuterClass",
+      "initializer": {
+        "docs": {
+          "stability": "experimental"
+        }
+      },
+      "kind": "class",
+      "locationInModule": {
+        "filename": "lib/submodule/child/index.ts",
+        "line": 24
+      },
+      "name": "OuterClass",
+      "namespace": "submodule.child",
+      "properties": [
+        {
+          "docs": {
+            "stability": "experimental"
+          },
+          "immutable": true,
+          "locationInModule": {
+            "filename": "lib/submodule/child/index.ts",
+            "line": 25
+          },
+          "name": "innerClass",
+          "type": {
+            "fqn": "jsii-calc.submodule.child.InnerClass"
+          }
+        }
+      ]
+    },
+    "jsii-calc.submodule.child.SomeEnum": {
+      "assembly": "jsii-calc",
+      "docs": {
+        "stability": "experimental"
+      },
+      "fqn": "jsii-calc.submodule.child.SomeEnum",
+      "kind": "enum",
+      "locationInModule": {
+        "filename": "lib/submodule/child/index.ts",
+        "line": 31
+      },
+      "members": [
+        {
+          "docs": {
+            "stability": "experimental"
+          },
+          "name": "SOME"
+        }
+      ],
+      "name": "SomeEnum",
+      "namespace": "submodule.child"
+    },
+    "jsii-calc.submodule.child.SomeStruct": {
+      "assembly": "jsii-calc",
+      "datatype": true,
+      "docs": {
+        "stability": "experimental"
+      },
+      "fqn": "jsii-calc.submodule.child.SomeStruct",
+      "kind": "interface",
+      "locationInModule": {
+        "filename": "lib/submodule/child/index.ts",
+        "line": 34
+      },
+      "name": "SomeStruct",
+      "namespace": "submodule.child",
+      "properties": [
+        {
+          "abstract": true,
+          "docs": {
+            "stability": "experimental"
+          },
+          "immutable": true,
+          "locationInModule": {
+            "filename": "lib/submodule/child/index.ts",
+            "line": 35
+          },
+          "name": "prop",
+          "type": {
+            "fqn": "jsii-calc.submodule.child.SomeEnum"
+          }
+        }
+      ]
+    },
     "jsii-calc.submodule.child.Structure": {
       "assembly": "jsii-calc",
       "datatype": true,
@@ -13000,5 +13128,5 @@
     }
   },
   "version": "0.0.0",
-  "fingerprint": "/DWCMji07IcZLGXOoefpKRPZyV9IHOIAR19tb1MHHWU="
+  "fingerprint": "5SbCJfv1kDW24DUhPcwzoQ1k7Moq+rKOB+Q4TSZlKPE="
 }
diff --git a/packages/jsii-pacmak/lib/targets/java.ts b/packages/jsii-pacmak/lib/targets/java.ts
@@ -2529,11 +2529,12 @@ class JavaGenerator extends Generator {
       'throw new ClassNotFoundException("Unknown JSII type: " + fqn);',
     );
     this.code.closeBlock();
-    this.code.line(
-      'return this.cache.computeIfAbsent(MODULE_TYPES.get(fqn), this::findClass);',
-    );
+    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));');
+    this.code.closeBlock();
+    this.code.line('return this.cache.get(className);');
     this.code.closeBlock();
-
     this.code.line();
     this.code.openBlock('private Class<?> findClass(final String binaryName)');
     this.code.openBlock('try');

diff --git a/...-of-base/java/src/main/java/software/amazon/jsii/tests/calculator/baseofbase/$Module.java b/...-of-base/java/src/main/java/software/amazon/jsii/tests/calculator/baseofbase/$Module.java
@@ -49,7 +49,11 @@ protected Class<?> resolveClass(final String fqn) throws ClassNotFoundException
         if (!MODULE_TYPES.containsKey(fqn)) {
             throw new ClassNotFoundException("Unknown JSII type: " + fqn);
         }
-        return this.cache.computeIfAbsent(MODULE_TYPES.get(fqn), this::findClass);
+        String className = MODULE_TYPES.get(fqn);
+        if (!this.cache.containsKey(className)) {
+            this.cache.put(className, this.findClass(className));
+        }
+        return this.cache.get(className);
     }
 
     private Class<?> findClass(final String binaryName) {

diff --git a/...jsii-calc-base/java/src/main/java/software/amazon/jsii/tests/calculator/base/$Module.java b/...jsii-calc-base/java/src/main/java/software/amazon/jsii/tests/calculator/base/$Module.java
@@ -57,7 +57,11 @@ protected Class<?> resolveClass(final String fqn) throws ClassNotFoundException
         if (!MODULE_TYPES.containsKey(fqn)) {
             throw new ClassNotFoundException("Unknown JSII type: " + fqn);
         }
-        return this.cache.computeIfAbsent(MODULE_TYPES.get(fqn), this::findClass);
+        String className = MODULE_TYPES.get(fqn);
+        if (!this.cache.containsKey(className)) {
+            this.cache.put(className, this.findClass(className));
+        }
+        return this.cache.get(className);
     }
 
     private Class<?> findClass(final String binaryName) {

diff --git a/...d.jsii-calc-lib/java/src/main/java/software/amazon/jsii/tests/calculator/lib/$Module.java b/...d.jsii-calc-lib/java/src/main/java/software/amazon/jsii/tests/calculator/lib/$Module.java
@@ -57,7 +57,11 @@ protected Class<?> resolveClass(final String fqn) throws ClassNotFoundException
         if (!MODULE_TYPES.containsKey(fqn)) {
             throw new ClassNotFoundException("Unknown JSII type: " + fqn);
         }
-        return this.cache.computeIfAbsent(MODULE_TYPES.get(fqn), this::findClass);
+        String className = MODULE_TYPES.get(fqn);
+        if (!this.cache.containsKey(className)) {
+            this.cache.put(className, this.findClass(className));
+        }
+        return this.cache.get(className);
     }
 
     private Class<?> findClass(final String binaryName) {