Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/20179#discussion_r160312383
--- Diff: core/src/main/scala/org/apache/spark/rpc/netty/NettyRpcEnv.scala
---
@@ -332,15 +332,17 @@ private[netty] class NettyRpcEnv(
val pipe = Pipe.open()
val source = new FileDownloadChannel(pipe.source())
+ var exceptionThrown = true
try {
val client = downloadClient(parsedUri.getHost(), parsedUri.getPort())
val callback = new FileDownloadCallback(pipe.sink(), source, client)
client.stream(parsedUri.getPath(), callback)
- } catch {
- case e: Exception =>
+ exceptionThrown = false
+ } finally {
+ if (exceptionThrown) {
pipe.sink().close()
--- End diff --
To preserve the same behavior, I think we'd need to do
```scala
try {
// code
} catch {
case t: Throwable =>
pipe.sink().close()
source.close()
throw t
}
```
to ensure that we propagate the original `Throwable`.
It could be clearer (and safer, in the case of exceptions thrown from
`close()` calls), to use
[`Utils.tryWithSafeFinallyAndFailureCallbacks`](https://github.com/apache/spark/blob/849043ce1d28a976659278d29368da0799329db8/core/src/main/scala/org/apache/spark/util/Utils.scala#L1407)
here, so let me make that change.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
