[SPARK-22058][CORE]the BufferedInputStream will not be closed if an exception occurs. by zuotingbing · Pull Request #19277 · apache/spark

zuotingbing · 2017-09-19T11:28:39Z

What changes were proposed in this pull request?

EventLoggingListener use val in = new BufferedInputStream(fs.open(log)) and will close it if codec.map(_.compressedInputStream(in)).getOrElse(in) occurs an exception .
But, if CompressionCodec.createCodec(new SparkConf, c) throws an exception, the BufferedInputStream in will not be closed anymore.

How was this patch tested?

exist tests

…xception occurs

srowen · 2017-09-19T11:30:35Z

core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala

+      // Compression codec is encoded as an extension, e.g. app_123.lzf
+      // Since we sanitize the app ID to not include periods, it is safe to split on it
+      val logName = log.getName.stripSuffix(IN_PROGRESS)
+      val codecName: Option[String] = logName.split("\\.").tail.lastOption


These two lines don't need to be in the try block. Best to keep it small.

yes, will fix it. Thanks srowen.

jerryshao · 2017-09-20T03:18:09Z

core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala

-
+
    try {
+      val codec = codecName.map { c =>


Why would here throw an exception?

Because there can throws an exception with "Codec [$codecName] is not available" in CompressionCodec.createCodec function.

While we're tightening, this whole body could be

codecName.map { c => val codec = codecMap.getOrElseUpdate(c, CompressionCodec.createCodec(new SparkConf, c)) codec.compressedInputStream(in) }.getOrElse(in)

jerryshao · 2017-09-20T03:18:51Z

core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala

+      val codec = codecName.map { c =>
+        codecMap.getOrElseUpdate(c, CompressionCodec.createCodec(new SparkConf, c))
+      }
      codec.map(_.compressedInputStream(in)).getOrElse(in)


Is it better to move this line val in = new BufferedInputStream(fs.open(log)) to here to solve your problem?

zuotingbing · 2017-09-20T07:18:08Z

I am not sure. If we move this line val in = new BufferedInputStream(fs.open(log)) into try~catch, we should define var in: BufferedInputStream = null before, and use catch { case e: Exception => if (in != null) { in.close() in = null } throw e } to close the BufferedInputStream when exception occurs. It is more complex, so i would like the code before.

jerryshao · 2017-09-20T07:24:17Z

Strictly saying, this line new BufferedInputStream(fs.open(log)) will also throw exception, shouldn't you try-catch it?

srowen · 2017-09-20T07:32:46Z

core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala

-
+
    try {
+      val codec = codecName.map { c =>


While we're tightening, this whole body could be

codecName.map { c => val codec = codecMap.getOrElseUpdate(c, CompressionCodec.createCodec(new SparkConf, c)) codec.compressedInputStream(in) }.getOrElse(in)

srowen · 2017-09-20T07:33:12Z

core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala

@@ -351,11 +351,11 @@ private[spark] object EventLoggingListener extends Logging {
    // Since we sanitize the app ID to not include periods, it is safe to split on it
    val logName = log.getName.stripSuffix(IN_PROGRESS)


Maybe move declaration of in to just before the try

srowen · 2017-09-20T07:33:55Z

core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala

+      }
      codec.map(_.compressedInputStream(in)).getOrElse(in)
    } catch {
      case e: Exception =>


I also note this doesn't handle Throwable, nor does a similar block earlier in the file. In case of say OutOfMemoryError it wouldn't be closed

yes , but if occurs error , it is of small significance to close the BufferedInputStream.
will fix as you said.

zuotingbing · 2017-09-20T07:48:18Z

@jerryshao if this line new BufferedInputStream(fs.open(log)) throws exception, it mean we do not need to close the object of BufferedInputStream because it new failed.

srowen · 2017-09-20T08:10:39Z

@zuotingbing no, fs.open could succeed, but new BufferedInputStream could fail, leaving the underlying stream open. In practice, it can't actually fail because this constructor does no I/O or operations that can fail. You could get away with leaving it as is.

jiangxb1987 · 2017-09-20T18:49:24Z

core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala

      codec.map(_.compressedInputStream(in)).getOrElse(in)
    } catch {
-      case e: Exception =>
+      case e: Throwable =>


What specific case do we want to catch here?

srowen 19 hours ago Member
I also note this doesn't handle Throwable, nor does a similar block earlier in the file. In case of say OutOfMemoryError it wouldn't be closed

with srowen's suggest, we could handle everything include exceptions and errors.

srowen · 2017-09-21T07:47:17Z

What other instances of try-catch might need improvement like this?

SparkQA · 2017-09-22T07:36:33Z

Test build #3929 has finished for PR 19277 at commit e3f8e0d.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-09-22T15:19:10Z

Test build #3932 has started for PR 19277 at commit 2e5f21a.

SparkQA · 2017-09-23T17:49:59Z

Test build #3933 has finished for PR 19277 at commit 2e5f21a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

[SPARK-22058][CORE]the BufferedInputStream will not be closed if an e…

ff4c449

…xception occurs

srowen requested changes Sep 19, 2017

View reviewed changes

fix code review

824856b

zuotingbing changed the title ~~[SPARK-22058][COREthe BufferedInputStream will not be closed if an exception occurs.~~ [SPARK-22058][CORE]the BufferedInputStream will not be closed if an exception occurs. Sep 19, 2017

jerryshao reviewed Sep 20, 2017

View reviewed changes

srowen reviewed Sep 20, 2017

View reviewed changes

handle Throwable

e3f8e0d

jiangxb1987 reviewed Sep 20, 2017

View reviewed changes

fix Scala style checks

2e5f21a

asfgit closed this in 4943ea5 Sep 24, 2017

zuotingbing deleted the SPARK-22058 branch September 25, 2017 01:18

		@@ -351,11 +351,11 @@ private[spark] object EventLoggingListener extends Logging {
		// Since we sanitize the app ID to not include periods, it is safe to split on it
		val logName = log.getName.stripSuffix(IN_PROGRESS)

Conversation

zuotingbing commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuotingbing Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuotingbing commented Sep 20, 2017

Uh oh!

jerryshao commented Sep 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuotingbing commented Sep 20, 2017

Uh oh!

srowen commented Sep 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuotingbing Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srowen commented Sep 21, 2017

Uh oh!

SparkQA commented Sep 22, 2017

Uh oh!

SparkQA commented Sep 22, 2017

Uh oh!

SparkQA commented Sep 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zuotingbing commented Sep 19, 2017 •

edited

Loading

zuotingbing Sep 20, 2017 •

edited

Loading

zuotingbing Sep 21, 2017 •

edited

Loading