Github user squito commented on a diff in the pull request:
https://github.com/apache/spark/pull/22557#discussion_r221056816
--- Diff:
core/src/main/scala/org/apache/spark/security/CryptoStreamUtils.scala ---
@@ -157,6 +165,111 @@ private[spark] object CryptoStreamUtils extends
Logging {
}
+ /**
+ * SPARK-25535. The commons-cryto library will throw InternalError if
something goes
+ * wrong, and leave bad state behind in the Java wrappers, so it's not
safe to use them
+ * afterwards. This wrapper detects that situation and avoids further
calls into the
+ * commons-crypto code, while still allowing the underlying streams to
be closed.
+ *
+ * This should be removed once CRYPTO-141 is fixed (and Spark upgrades
its commons-crypto
+ * dependency).
+ */
+ trait BaseErrorHandler extends Closeable {
+
+ private var closed = false
+
+ protected def cipherStream: Closeable
+ protected def wrapped: Closeable
--- End diff --
its a little confusing that there are two closeables. I'd add a comment
here that `wrapped` is what the `cipherStream` is already wrapping, and we want
to make sure it gets closed if the `cipherStream` has an internal error.
(when I see a variable named `wrapped`, I assumed it was what *this* was
wrapping, not what the cipherStream was wrapping)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
