Skip to content

Commit fafb0fd

Browse files
zhangskzcopybara-github
authored andcommitted
Fix Java concurrency issue in feature resolution for old <=3.25.x gencode using lazy feature resolution.
This should not impact current gencode from >=4.x.x which resolves features in static init. Fixes #20599 PiperOrigin-RevId: 738467326
1 parent 213c512 commit fafb0fd

File tree

3 files changed

+121
-6
lines changed

3 files changed

+121
-6
lines changed

java/core/BUILD.bazel

+1
Original file line numberDiff line numberDiff line change
@@ -533,6 +533,7 @@ LITE_TEST_EXCLUSIONS = [
533533
"src/test/java/com/google/protobuf/CodedInputStreamTest.java",
534534
"src/test/java/com/google/protobuf/DeprecatedFieldTest.java",
535535
"src/test/java/com/google/protobuf/DebugFormatTest.java",
536+
"src/test/java/com/google/protobuf/ConcurrentDescriptorsTest.java",
536537
"src/test/java/com/google/protobuf/DescriptorsTest.java",
537538
"src/test/java/com/google/protobuf/DiscardUnknownFieldsTest.java",
538539
"src/test/java/com/google/protobuf/DynamicMessageTest.java",

java/core/src/main/java/com/google/protobuf/Descriptors.java

+7-6
Original file line numberDiff line numberDiff line change
@@ -537,6 +537,7 @@ public interface InternalDescriptorAssigner {
537537
private final FileDescriptor[] dependencies;
538538
private final FileDescriptor[] publicDependencies;
539539
private final DescriptorPool pool;
540+
private boolean featuresResolved;
540541

541542
private FileDescriptor(
542543
final FileDescriptorProto proto,
@@ -547,6 +548,7 @@ private FileDescriptor(
547548
this.pool = pool;
548549
this.proto = proto;
549550
this.dependencies = dependencies.clone();
551+
this.featuresResolved = false;
550552
HashMap<String, FileDescriptor> nameToFileMap = new HashMap<>();
551553
for (FileDescriptor file : dependencies) {
552554
nameToFileMap.put(file.getName(), file);
@@ -618,6 +620,7 @@ private FileDescriptor(
618620
.build();
619621
this.dependencies = new FileDescriptor[0];
620622
this.publicDependencies = new FileDescriptor[0];
623+
this.featuresResolved = false;
621624

622625
messageTypes = new Descriptor[] {message};
623626
enumTypes = EMPTY_ENUM_DESCRIPTORS;
@@ -641,12 +644,12 @@ public void resolveAllFeaturesImmutable() {
641644
* and all of its children.
642645
*/
643646
private void resolveAllFeaturesInternal() throws DescriptorValidationException {
644-
if (this.features != null) {
647+
if (this.featuresResolved) {
645648
return;
646649
}
647650

648651
synchronized (this) {
649-
if (this.features != null) {
652+
if (this.featuresResolved) {
650653
return;
651654
}
652655
resolveFeatures(proto.getOptions().getFeatures());
@@ -666,6 +669,7 @@ private void resolveAllFeaturesInternal() throws DescriptorValidationException {
666669
for (FieldDescriptor extension : extensions) {
667670
extension.resolveAllFeatures();
668671
}
672+
this.featuresResolved = true;
669673
}
670674
}
671675

@@ -2934,10 +2938,7 @@ FeatureSet getFeatures() {
29342938
}
29352939
if (this.features == null) {
29362940
throw new NullPointerException(
2937-
String.format(
2938-
"Features not yet loaded for %s. This may be caused by a known issue for proto2"
2939-
+ " dependency descriptors obtained from proto1 (b/362326130)",
2940-
getFullName()));
2941+
String.format("Features not yet loaded for %s.", getFullName()));
29412942
}
29422943
return this.features;
29432944
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
// Protocol Buffers - Google's data interchange format
2+
// Copyright 2025 Google Inc. All rights reserved.
3+
//
4+
// Use of this source code is governed by a BSD-style
5+
// license that can be found in the LICENSE file or at
6+
// https://developers.google.com/open-source/licenses/bsd
7+
8+
package com.google.protobuf;
9+
10+
import proto2_unittest.UnittestProto;
11+
import proto2_unittest.UnittestProto.TestAllTypes;
12+
import java.util.ArrayList;
13+
import java.util.List;
14+
import java.util.concurrent.CountDownLatch;
15+
import java.util.concurrent.ExecutionException;
16+
import java.util.concurrent.ExecutorService;
17+
import java.util.concurrent.Executors;
18+
import java.util.concurrent.Future;
19+
import org.junit.Assert;
20+
import org.junit.Test;
21+
import org.junit.runner.RunWith;
22+
import org.junit.runners.JUnit4;
23+
24+
@RunWith(JUnit4.class)
25+
public final class ConcurrentDescriptorsTest {
26+
public static final int N = 1000;
27+
28+
static class Worker implements Runnable {
29+
private final CountDownLatch startSignal;
30+
private final CountDownLatch doneSignal;
31+
private final Runnable trigger;
32+
33+
Worker(CountDownLatch startSignal, CountDownLatch doneSignal, Runnable trigger) {
34+
this.startSignal = startSignal;
35+
this.doneSignal = doneSignal;
36+
this.trigger = trigger;
37+
}
38+
39+
@Override
40+
public void run() {
41+
try {
42+
startSignal.await();
43+
trigger.run();
44+
} catch (InterruptedException | RuntimeException e) {
45+
doneSignal.countDown();
46+
throw new RuntimeException(e); // Rethrow for main thread to handle
47+
}
48+
doneSignal.countDown();
49+
}
50+
}
51+
52+
@Test
53+
public void testSimultaneousStaticInit() throws InterruptedException {
54+
ExecutorService executor = Executors.newFixedThreadPool(N);
55+
CountDownLatch startSignal = new CountDownLatch(1);
56+
CountDownLatch doneSignal = new CountDownLatch(N);
57+
List<Future<?>> futures = new ArrayList<>(N);
58+
for (int i = 0; i < N; i++) {
59+
Future<?> future =
60+
executor.submit(
61+
new Worker(
62+
startSignal,
63+
doneSignal,
64+
// Static method invocation triggers static initialization.
65+
() -> Assert.assertNotNull(UnittestProto.getDescriptor())));
66+
futures.add(future);
67+
}
68+
startSignal.countDown();
69+
doneSignal.await();
70+
System.out.println("Done with all threads...");
71+
for (int i = 0; i < futures.size(); i++) {
72+
try {
73+
futures.get(i).get();
74+
} catch (ExecutionException e) {
75+
Assert.fail("Thread " + i + " failed with:" + e.getMessage());
76+
}
77+
}
78+
executor.shutdown();
79+
}
80+
81+
@Test
82+
public void testSimultaneousFeatureAccess() throws InterruptedException {
83+
ExecutorService executor = Executors.newFixedThreadPool(N);
84+
CountDownLatch startSignal = new CountDownLatch(1);
85+
CountDownLatch doneSignal = new CountDownLatch(N);
86+
List<Future<?>> futures = new ArrayList<>(N);
87+
for (int i = 0; i < N; i++) {
88+
Future<?> future =
89+
executor.submit(
90+
new Worker(
91+
startSignal,
92+
doneSignal,
93+
// hasPresence() uses the [field_presence] feature.
94+
() ->
95+
Assert.assertTrue(
96+
TestAllTypes.getDescriptor()
97+
.findFieldByName("optional_int32")
98+
.hasPresence())));
99+
futures.add(future);
100+
}
101+
startSignal.countDown();
102+
doneSignal.await();
103+
System.out.println("Done with all threads...");
104+
for (int i = 0; i < futures.size(); i++) {
105+
try {
106+
futures.get(i).get();
107+
} catch (ExecutionException e) {
108+
Assert.fail("Thread " + i + " failed with:" + e.getMessage());
109+
}
110+
}
111+
executor.shutdown();
112+
}
113+
}

0 commit comments

Comments
 (0)