gemini-code-assist[bot] commented on code in PR #38906:
URL: https://github.com/apache/beam/pull/38906#discussion_r3395423607
##########
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoByteBuddyUtils.java:
##########
@@ -546,7 +546,7 @@ public TypeConversion<StackManipulation>
createSetterConversions(StackManipulati
return builder
.visit(new
AsmVisitorWrapper.ForDeclaredMethods().writerFlags(ClassWriter.COMPUTE_FRAMES))
.make()
- .load(ReflectHelpers.findClassLoader(),
ClassLoadingStrategy.Default.INJECTION)
+ .load(ReflectHelpers.findClassLoader(protoClass),
getClassLoadingStrategy(protoClass))
Review Comment:
### Potential Class Loader Visibility Issues and `sun.misc.Unsafe` Fallback
Using `getClassLoadingStrategy(protoClass)` and
`ReflectHelpers.findClassLoader(protoClass)` introduces potential runtime
issues and may prevent fully eliminating the dependency on `sun.misc.Unsafe`:
1. **Class Loader Visibility (`NoClassDefFoundError`):**
If `protoClass` is loaded by a parent class loader (e.g., a shared
library class loader in Spark/Flink or the platform class loader for standard
protobuf classes) that cannot see Beam classes, attempting to load the
generated class (which implements Beam's `FieldValueSetter` or
`SchemaUserTypeCreator`) into `protoClass`'s class loader will fail with a
`NoClassDefFoundError` during class resolution.
2. **Fallback to `INJECTION` (and `Unsafe`):**
If `getClassLoadingStrategy(protoClass)` detects that it cannot use
`UsingLookup` (due to module access or class loader restrictions), it falls
back to `ClassLoadingStrategy.Default.INJECTION`, which relies on
`sun.misc.Unsafe`. This defeats the goal of completely preparing for the
deprecation of `sun.misc.Unsafe`.
### Recommended Alternative
Since all accessed members of the protobuf classes are public, the generated
helper classes do not need to reside in the same package as `protoClass`.
Consider the following refactoring:
1. Name the generated classes within Beam's package (e.g.,
`org.apache.beam.sdk.extensions.protobuf`).
2. Load them using Beam's class loader:
`ProtoByteBuddyUtils.class.getClassLoader()`.
3. Use `getClassLoadingStrategy(ProtoByteBuddyUtils.class)` which will
safely resolve to `UsingLookup` on `ProtoByteBuddyUtils.class` without any risk
of `NoClassDefFoundError` or needing to fall back to `INJECTION`.
This applies to all three locations in this file (lines 549, 601-603, and
1121-1123).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
