From ab16afc250941fe9e956b2131d436c9674bd9724 Mon Sep 17 00:00:00 2001 From: Rubin Yoo Date: Wed, 1 Oct 2025 16:25:17 -0700 Subject: [PATCH] The null field initialization code has a race condition, and this fix ensures correctness by checking the appropriate variable. --- .../motif/compiler/JavaCodeGenerator.kt | 5 +- .../motif/compiler/KotlinCodeGenerator.kt | 3 +- samples/sample/build.gradle | 4 +- .../GRAPH.txt | 32 +++++++++ .../Scope.kt | 32 +++++++++ .../Test.java | 71 +++++++++++++++++++ .../GRAPH.txt | 32 +++++++++ .../Scope.java | 35 +++++++++ .../Test.java | 65 +++++++++++++++++ 9 files changed, 274 insertions(+), 5 deletions(-) create mode 100644 tests/src/main/java/testcases/KT009_use_null_field_concurrency_kotlin/GRAPH.txt create mode 100644 tests/src/main/java/testcases/KT009_use_null_field_concurrency_kotlin/Scope.kt create mode 100644 tests/src/main/java/testcases/KT009_use_null_field_concurrency_kotlin/Test.java create mode 100644 tests/src/main/java/testcases/T078_use_null_field_concurrency_java/GRAPH.txt create mode 100644 tests/src/main/java/testcases/T078_use_null_field_concurrency_java/Scope.java create mode 100644 tests/src/main/java/testcases/T078_use_null_field_concurrency_java/Test.java diff --git a/compiler/src/main/kotlin/motif/compiler/JavaCodeGenerator.kt b/compiler/src/main/kotlin/motif/compiler/JavaCodeGenerator.kt index 739821fe..30c56e05 100644 --- a/compiler/src/main/kotlin/motif/compiler/JavaCodeGenerator.kt +++ b/compiler/src/main/kotlin/motif/compiler/JavaCodeGenerator.kt @@ -194,7 +194,8 @@ object JavaCodeGenerator { .add("Object $localFieldName = \$N;\n", cacheFieldName) .beginControlFlow("if (\$N == null)", localFieldName) .beginControlFlow("synchronized (this)") - .beginControlFlow("if (\$N == null)", cacheFieldName) + .add("\$N = \$N;\n", localFieldName, cacheFieldName) + .beginControlFlow("if (\$N == null)", localFieldName) .add("\$N = \$L;\n", localFieldName, instantiation.spec()) .beginControlFlow("if (\$N == null)", localFieldName) .add( @@ -203,7 +204,7 @@ object JavaCodeGenerator { "Factory method cannot return null", ) .endControlFlow() - .add("\$N = \$L;\n", cacheFieldName, localFieldName) + .add("\$N = \$N;\n", cacheFieldName, localFieldName) .endControlFlow() .endControlFlow() .endControlFlow() diff --git a/compiler/src/main/kotlin/motif/compiler/KotlinCodeGenerator.kt b/compiler/src/main/kotlin/motif/compiler/KotlinCodeGenerator.kt index fe0f205b..4bdc2654 100644 --- a/compiler/src/main/kotlin/motif/compiler/KotlinCodeGenerator.kt +++ b/compiler/src/main/kotlin/motif/compiler/KotlinCodeGenerator.kt @@ -231,7 +231,8 @@ object KotlinCodeGenerator { .addStatement("var $localFieldName = %N;\n", cacheFieldName) .beginControlFlow("if (%N == null)", localFieldName) .beginControlFlow("synchronized (this)") - .beginControlFlow("if (%N == null)", cacheFieldName) + .addStatement("%N = %N", localFieldName, cacheFieldName) + .beginControlFlow("if (%N == null)", localFieldName) .addStatement("%N = %L", localFieldName, instantiation.spec()) .addStatement("%N = %N", cacheFieldName, localFieldName) .endControlFlow() diff --git a/samples/sample/build.gradle b/samples/sample/build.gradle index 1607a488..8ef8443d 100644 --- a/samples/sample/build.gradle +++ b/samples/sample/build.gradle @@ -12,8 +12,8 @@ android { } compileOptions { - sourceCompatibility JavaVersion.VERSION_1_8 - targetCompatibility JavaVersion.VERSION_1_8 + sourceCompatibility JavaVersion.VERSION_11 + targetCompatibility JavaVersion.VERSION_11 } defaultConfig { diff --git a/tests/src/main/java/testcases/KT009_use_null_field_concurrency_kotlin/GRAPH.txt b/tests/src/main/java/testcases/KT009_use_null_field_concurrency_kotlin/GRAPH.txt new file mode 100644 index 00000000..325f6fba --- /dev/null +++ b/tests/src/main/java/testcases/KT009_use_null_field_concurrency_kotlin/GRAPH.txt @@ -0,0 +1,32 @@ +######################################################################## +# # +# This file is auto-generated by running the Motif compiler tests and # +# serves a as validation of graph correctness. IntelliJ plugin tests # +# also rely on this file to ensure that the plugin graph understanding # +# is equivalent to the compiler's. # +# # +# - Do not edit manually. # +# - Commit changes to source control. # +# - Since this file is autogenerated, code review changes carefully to # +# ensure correctness. # +# # +######################################################################## + + ------- +| Scope | + ------- + + ==== Required ==== + + ==== Provides ==== + + ---- Object | Objects.fooObject ---- + [ Required ] + [ Consumed By ] + * Scope | Scope.fooObject() + + ---- Scope | implicit ---- + [ Required ] + [ Consumed By ] + + diff --git a/tests/src/main/java/testcases/KT009_use_null_field_concurrency_kotlin/Scope.kt b/tests/src/main/java/testcases/KT009_use_null_field_concurrency_kotlin/Scope.kt new file mode 100644 index 00000000..9a0ef8f2 --- /dev/null +++ b/tests/src/main/java/testcases/KT009_use_null_field_concurrency_kotlin/Scope.kt @@ -0,0 +1,32 @@ +/* + * Copyright (c) 2018-2019 Uber Technologies, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package testcases.KT009_use_null_field_concurrency_kotlin + +import motif.Creatable + +@motif.Scope(useNullFieldInitialization = true) +interface Scope : Creatable { + fun fooObject(): Any + + @motif.Objects + abstract class Objects { + fun fooObject(): Any { + return Any() + } + } + + interface Dependencies +} \ No newline at end of file diff --git a/tests/src/main/java/testcases/KT009_use_null_field_concurrency_kotlin/Test.java b/tests/src/main/java/testcases/KT009_use_null_field_concurrency_kotlin/Test.java new file mode 100644 index 00000000..30a1e9c3 --- /dev/null +++ b/tests/src/main/java/testcases/KT009_use_null_field_concurrency_kotlin/Test.java @@ -0,0 +1,71 @@ +/* + * Copyright (c) 2018-2019 Uber Technologies, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package testcases.KT009_use_null_field_concurrency_kotlin; + +import static com.google.common.truth.Truth.assertThat; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; + + +public class Test { + + /** + * This tests if the ScopeImpl synchronized blocks check and assign correct values + */ + public static void run() { + Scope scope = new ScopeImpl(); + int nThreads = 2; + ExecutorService executorService = Executors.newFixedThreadPool(nThreads); + CountDownLatch getFooObjectLatch = new CountDownLatch(nThreads); + CountDownLatch end = new CountDownLatch(nThreads); + AtomicBoolean isFooObjectNull = new AtomicBoolean(false); + try { + synchronized (scope) { // blocks the synchronized in scope.fooObject + for (int i = 0; i < nThreads; i++) { + executorService.submit(() -> { + getFooObjectLatch.countDown(); + try { + Object fooObject = scope.fooObject(); + if(fooObject == null) { + isFooObjectNull.set(true); + } + } catch (Exception e) { + isFooObjectNull.set(true); + } + end.countDown(); + }); + } + getFooObjectLatch.await(1000, TimeUnit.MILLISECONDS); + } + // at this point, the two threads will compete to create the fooObject + + // Verify + if(end.await(1000, TimeUnit.MILLISECONDS)) { + assertThat(isFooObjectNull.get()).isFalse(); + } + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + } +} diff --git a/tests/src/main/java/testcases/T078_use_null_field_concurrency_java/GRAPH.txt b/tests/src/main/java/testcases/T078_use_null_field_concurrency_java/GRAPH.txt new file mode 100644 index 00000000..325f6fba --- /dev/null +++ b/tests/src/main/java/testcases/T078_use_null_field_concurrency_java/GRAPH.txt @@ -0,0 +1,32 @@ +######################################################################## +# # +# This file is auto-generated by running the Motif compiler tests and # +# serves a as validation of graph correctness. IntelliJ plugin tests # +# also rely on this file to ensure that the plugin graph understanding # +# is equivalent to the compiler's. # +# # +# - Do not edit manually. # +# - Commit changes to source control. # +# - Since this file is autogenerated, code review changes carefully to # +# ensure correctness. # +# # +######################################################################## + + ------- +| Scope | + ------- + + ==== Required ==== + + ==== Provides ==== + + ---- Object | Objects.fooObject ---- + [ Required ] + [ Consumed By ] + * Scope | Scope.fooObject() + + ---- Scope | implicit ---- + [ Required ] + [ Consumed By ] + + diff --git a/tests/src/main/java/testcases/T078_use_null_field_concurrency_java/Scope.java b/tests/src/main/java/testcases/T078_use_null_field_concurrency_java/Scope.java new file mode 100644 index 00000000..9e3c109c --- /dev/null +++ b/tests/src/main/java/testcases/T078_use_null_field_concurrency_java/Scope.java @@ -0,0 +1,35 @@ +/* + * Copyright (c) 2018-2019 Uber Technologies, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package testcases.T078_use_null_field_concurrency_java; + +import motif.Creatable; + +@motif.Scope(useNullFieldInitialization = true) +public interface Scope extends Creatable { + + Object fooObject(); + + @motif.Objects + class Objects { + + Object fooObject() { + return new Object(); + } + + } + + interface Dependencies {} +} \ No newline at end of file diff --git a/tests/src/main/java/testcases/T078_use_null_field_concurrency_java/Test.java b/tests/src/main/java/testcases/T078_use_null_field_concurrency_java/Test.java new file mode 100644 index 00000000..6085a7ab --- /dev/null +++ b/tests/src/main/java/testcases/T078_use_null_field_concurrency_java/Test.java @@ -0,0 +1,65 @@ +/* + * Copyright (c) 2018-2019 Uber Technologies, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package testcases.T078_use_null_field_concurrency_java; + +import static com.google.common.truth.Truth.assertThat; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; + +public class Test { + + /** + * This tests if the ScopeImpl synchronized blocks check and assign correct values + */ + public static void run() { + Scope scope = new ScopeImpl(); + int nThreads = 2; + ExecutorService executorService = Executors.newFixedThreadPool(nThreads); + CountDownLatch getFooObjectLatch = new CountDownLatch(nThreads); + CountDownLatch end = new CountDownLatch(nThreads); + AtomicBoolean isFooObjectNull = new AtomicBoolean(false); + try { + synchronized (scope) { // blocks the synchronized in scope.fooObject + for (int i = 0; i < nThreads; i++) { + executorService.submit(() -> { + getFooObjectLatch.countDown(); + Object fooObject = scope.fooObject(); + if(fooObject == null) { + isFooObjectNull.set(true); + } + end.countDown(); + }); + } + getFooObjectLatch.await(1000, TimeUnit.MILLISECONDS); + } + // at this point, the two threads will compete to create the fooObject + + // Verify + if(end.await(1000, TimeUnit.MILLISECONDS)) { + assertThat(isFooObjectNull.get()).isFalse(); + } + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + } +}