xiaoyanxie commented on code in PR #2395:
URL: https://github.com/apache/auron/pull/2395#discussion_r3817713119
##########
spark-extension/src/main/scala/org/apache/spark/sql/auron/NativeConverters.scala:
##########
@@ -1505,24 +1507,63 @@ object NativeConverters extends Logging {
}
}
+ /**
+ * ObjectInputStream that resolves classes against an explicit class loader.
+ *
+ * The default ObjectInputStream.resolveClass resolves each class through
+ * VM.latestUserDefinedLoader(), which selects a loader from the live call
stack rather than the
+ * context class loader. During a nested read the most recent user-defined
frame is often a
+ * Spark or Scala class, whose loader cannot see Auron classes when Auron is
supplied through
+ * spark.jars and therefore loaded by MutableURLClassLoader. The expression
graph then resolves
+ * only partially and an un-readResolve'd DefaultSerializationProxy is
assigned into
+ * RDD.dependencies_, raising a ClassCastException. Pinning the loader keeps
resolution
+ * independent of the call stack. Spark's own JavaDeserializationStream does
the same.
+ */
+ private class AuronObjectInputStream(in: InputStream, loader: ClassLoader)
+ extends ObjectInputStream(in) {
+
+ // scalastyle:off classforname
+ private def load(name: String, cl: ClassLoader): Class[_] =
Class.forName(name, false, cl)
+ // scalastyle:on classforname
+
+ // resolveProxyClass is deliberately not overridden: the only
non-deprecated way to obtain a
+ // proxy Class is Proxy.getProxyClass, and serialized expressions contain
no dynamic proxies.
+ override def resolveClass(desc: ObjectStreamClass): Class[_] = {
Review Comment:
Thanks for the review. You are right about the `resolveProxyClass`.
I checked Spark's JavaDeserializationStream. It overrides both
`resolveClass` and `resolveProxyClass`, so the example I referenced actually
supports your point. And "no dynamic proxies" is not something we can
guarantee. `deserializeExpression` is called from
`SparkAuronUDFWrapperContext`, `SparkUDAFWrapperContext` and
`SparkUDTFWrapperContext`, which carry user payloads.
In the fix
[fa4047e](https://github.com/apache/auron/pull/2395/changes/fa4047e8d6da90f66ff96cb451ecda868d5a7ea6)
I moved the pinned-loader -> Auron-loader -> default fallback into a helper
method `withLoaderFallback`. Now both `resolveClass` and `resolveProxyClass`
use it. `Proxy.getProxyClass` is deprecated since Java 9, and our build uses
`-Xfatal-warnings`, so I also added a `-Wconf` suppression in pom.xml. This
follows the same way we already handle `Class.newInstance`.
I also added a test for this. The test redefines the proxy interface in a
child class loader, then checks which loader resolves the proxy after
deserialization. When I run this test against the code before the fix, it fails:
```
- deserializeExpression resolves proxy interfaces with the pinned class
loader *** FAILED ***
jdk.internal.loader.ClassLoaders$AppClassLoader@5ffd2b27 was not the same
instance as org.apache.auron.SingleClassRedefiningLoader@49353d43 proxy
interface was resolved by a class loader taken from the call stack
(jdk.internal.loader.ClassLoaders$AppClassLoader@5ffd2b27) instead of the
pinned loader (NativeConvertersSuite.scala:124)
```
After the fix, the test passes.
One question I would like your opinion on. The fallback chain has one cost
that the single-loader version did not have. If no loader can resolve a name,
we now throw ClassNotFoundException on each level. I measured some cases, for
example: a ScalaUDF with primitive type arguments will serialize
ExpressionEncoder, and its ClassTag holds int.class, long.class and so on, with
a payload that contains 8 primitive descriptors:
| | µs/op |
|--------------------------------------------|-------|
| master (plain ObjectInputStream) | 194 |
| this PR | 578 |
| this PR + a 9-entry primitive lookup table | 26 |
For normal payloads without primitives, both are about 11% faster than
master, because pinning the loader avoids the stack walk in
`VM.latestUserDefinedLoader()`.
I already have the primitive lookup table change, but I did not include it
here, because I want to keep this PR only for the correctness fix. Please let
me know if you prefer to add it in this PR, or I can send a separate one.
--
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]
