Resolve features directly in setProto instead of temporarily setting to null.

zhangskz · zhangskz · commit aea0e52ec2b5 · 2024-03-05T11:03:15.000-05:00
Avoid potential races with other threads reading features that do not share a lock while features are temporarily null.

Special handling for proto1 mutable should not actually be needed, since setProto doesn't update dependency protos.

PiperOrigin-RevId: 610783483
diff --git a/java/core/src/main/java/com/google/protobuf/Descriptors.java b/java/core/src/main/java/com/google/protobuf/Descriptors.java
@@ -482,20 +482,17 @@ public static FileDescriptor internalBuildGeneratedFileFrom(
       return internalBuildGeneratedFileFrom(descriptorDataParts, dependencies);
     }
 
-    public static void internalUpdateFileDescriptorImmutable(
+    /**
+     * This method is to be called by generated code only. It updates the FileDescriptorProto
+     * associated with the descriptor by parsing it again with the given ExtensionRegistry. This is
+     * needed to recognize custom options.
+     */
+    public static void internalUpdateFileDescriptor(
         FileDescriptor descriptor, ExtensionRegistry registry) {
-      internalUpdateFileDescriptor(descriptor, registry, false);
-    }
-
-    private static void internalUpdateFileDescriptor(
-        FileDescriptor descriptor, ExtensionRegistry registry, boolean mutable) {
       ByteString bytes = descriptor.proto.toByteString();
       try {
         FileDescriptorProto proto = FileDescriptorProto.parseFrom(bytes, registry);
-        synchronized (descriptor) {
-          descriptor.setProto(proto);
-          descriptor.resolveAllFeaturesImmutable();
-        }
+        descriptor.setProto(proto);
       } catch (InvalidProtocolBufferException e) {
         throw new IllegalArgumentException(
             "Failed to parse protocol buffer descriptor for generated code.", e);
@@ -712,10 +709,10 @@ private void crossLink() throws DescriptorValidationException {
      * construct the descriptors we have to have parsed the descriptor protos. So, we have to parse
      * the descriptor protos a second time after constructing the descriptors.
      */
-    private void setProto(final FileDescriptorProto proto) {
+    private synchronized void setProto(final FileDescriptorProto proto) {
       this.proto = proto;
-      this.features = null;
       this.options = null;
+      this.features = resolveFeatures(proto.getOptions().getFeatures());
 
       for (int i = 0; i < messageTypes.length; i++) {
         messageTypes[i].setProto(proto.getMessageType(i));
@@ -1167,8 +1164,8 @@ private void validateNoDuplicateFieldNumbers() throws DescriptorValidationExcept
     /** See {@link FileDescriptor#setProto}. */
     private void setProto(final DescriptorProto proto) {
       this.proto = proto;
-      this.features = null;
       this.options = null;
+      this.features = resolveFeatures(proto.getOptions().getFeatures());
 
       for (int i = 0; i < nestedTypes.length; i++) {
         nestedTypes[i].setProto(proto.getNestedType(i));
@@ -1983,8 +1980,8 @@ private void crossLink() throws DescriptorValidationException {
     /** See {@link FileDescriptor#setProto}. */
     private void setProto(final FieldDescriptorProto proto) {
       this.proto = proto;
-      this.features = null;
       this.options = null;
+      this.features = resolveFeatures(proto.getOptions().getFeatures());
     }
 
     /** For internal use only. This is to satisfy the FieldDescriptorLite interface. */
@@ -2263,8 +2260,8 @@ private void resolveAllFeatures() {
     /** See {@link FileDescriptor#setProto}. */
     private void setProto(final EnumDescriptorProto proto) {
       this.proto = proto;
-      this.features = null;
       this.options = null;
+      this.features = resolveFeatures(proto.getOptions().getFeatures());
 
       for (int i = 0; i < values.length; i++) {
         values[i].setProto(proto.getValue(i));
@@ -2412,8 +2409,8 @@ private void resolveAllFeatures() {
     /** See {@link FileDescriptor#setProto}. */
     private void setProto(final EnumValueDescriptorProto proto) {
       this.proto = proto;
-      this.features = null;
       this.options = null;
+      this.features = resolveFeatures(proto.getOptions().getFeatures());
     }
   }
 
@@ -2537,8 +2534,8 @@ private void crossLink() throws DescriptorValidationException {
     /** See {@link FileDescriptor#setProto}. */
     private void setProto(final ServiceDescriptorProto proto) {
       this.proto = proto;
-      this.features = null;
       this.options = null;
+      this.features = resolveFeatures(proto.getOptions().getFeatures());
 
       for (int i = 0; i < methods.length; i++) {
         methods[i].setProto(proto.getMethod(i));
@@ -2687,8 +2684,8 @@ private void crossLink() throws DescriptorValidationException {
     /** See {@link FileDescriptor#setProto}. */
     private void setProto(final MethodDescriptorProto proto) {
       this.proto = proto;
-      this.features = null;
       this.options = null;
+      this.features = resolveFeatures(proto.getOptions().getFeatures());
     }
   }
 
@@ -3223,8 +3220,8 @@ private void resolveAllFeatures() {
 
     private void setProto(final OneofDescriptorProto proto) {
       this.proto = proto;
-      this.features = null;
       this.options = null;
+      this.features = resolveFeatures(proto.getOptions().getFeatures());
     }
 
     private OneofDescriptor(
diff --git a/src/google/protobuf/compiler/java/file.cc b/src/google/protobuf/compiler/java/file.cc
@@ -490,7 +490,7 @@ void FileGenerator::GenerateDescriptorInitializationCodeForImmutable(
     }
     printer->Print(
         "com.google.protobuf.Descriptors.FileDescriptor\n"
-        "    .internalUpdateFileDescriptorImmutable(descriptor, registry);\n");
+        "    .internalUpdateFileDescriptor(descriptor, registry);\n");
   }
 
   printer->Outdent();

Original file line number	Diff line number	Diff line change
`@@ -490,7 +490,7 @@ void FileGenerator::GenerateDescriptorInitializationCodeForImmutable(`
`490`	`490`	`}`
`491`	`491`	`printer->Print(`
`492`	`492`	`"com.google.protobuf.Descriptors.FileDescriptor\n"`
`493`		`- " .internalUpdateFileDescriptorImmutable(descriptor, registry);\n");`
	`493`	`+ " .internalUpdateFileDescriptor(descriptor, registry);\n");`
`494`	`494`	`}`
`495`	`495`
`496`	`496`	`printer->Outdent();`