[SPARK-14914] Fix Resource not closed after using, mostly for unit tests #12693

taoli91 · 2016-04-26T07:54:55Z

What changes were proposed in this pull request?

Close FileStreams, ZipFiles etc to release the resources after using. Not closing the resources will cause IO Exception to be raised while deleting temp files.

How was this patch tested?

Run unit test on both windows and Linux. Note that this fix can't resolve all the problem of Unit tests on Windows.

Helping script for windows to download dependency and start zinc to support incremental building on windows.

… first stream

srowen · 2016-04-26T08:12:41Z

core/src/main/scala/org/apache/spark/rdd/ReliableCheckpointRDD.scala

@@ -243,7 +243,16 @@ private[spark] object ReliableCheckpointRDD extends Logging {
      if (fs.exists(partitionerFilePath)) {
        val fileInputStream = fs.open(partitionerFilePath, bufferSize)
        val serializer = SparkEnv.get.serializer.newInstance()
-        val deserializeStream = serializer.deserializeStream(fileInputStream)
+        // make sure that the file is closed if error occurrs during deserialization
+        val deserializeStream =


You should use finally in contexts like this.

@srowen The deserializeStream will be closed. We only need this when the deserializeStream method throws exception and we have no chance to go to the line 258, in which the deserializeStream will be closed, with the inner fileInputStream.

Sorry, I mean put this in the block below that closes the deserialization stream, at best.

Sorry, I mean put this in the block below that closes the deserialization stream, at best.

@srowen It seems we need extra code to accommodate the scope problem if putting the close in other clauses. Probably it's cleaner to stick with this solution.

I'm not sure what you mean -- it's less code. Just open the deserializeStream inside the block and close it in the finally block that follows. Actually, closing deserializeStream will already (should already) close the underlying stream anyway. It doesn't handle errors while making the stream from the original stream, but, constructors of the deserializer streams aren't reading the stream anyway. I suspect it's fine as-is, but it would be extra-defensive to also close the fileInputStream, yes.

@srowen Probably a code snippet will explain things better. Do you mean this:

var deserializeStream : DeserializationStream = null val partitioner = Utils.tryWithSafeFinally[Partitioner] { deserializeStream = serializer.deserializeStream(fileInputStream) deserializeStream.readObject[Partitioner] } { deserializeStream.close() }

There are a few things I am concerning

The semantic has slightly changed (deserializeStream is now var instead of val)

I'm not sure is it always safe to close an partially initialized deserializeStream, in the case of the deserialization throwing exception. I'm pretty sure that if we close the fileInputStream first may cause the closing deserializeSteam throwing exception complaining that the input has been closed already.

FYI, I found this problem by while running the test case "checkpointing partitioners" in which the corruptPartitionerFile flag is turned on in the CheckpointSuite.

Yeah that's what I mean. Well, if I were right that serializer.deserializeStream can't fail, then the existing code would already be fine, so this suggestion doesn't help unless fileInputStream gets a similar treatment, and that's too complex. I'm curious how that fails given the current implementation, but, even if it couldn't now, it could in the future.

You might normally resolve this with nested try blocks; I think streams aren't supposed to fail if closed twice, so, safe to close the underlying stream for good measure. Still at that point it's no less complex, so I can see that this is as clear as anything.

srowen · 2016-04-26T08:17:37Z

This cleanup looks generally good

HyukjinKwon · 2016-04-26T08:38:51Z

streaming/src/test/scala/org/apache/spark/streaming/MapWithStateSuite.scala

@@ -39,18 +39,15 @@ class MapWithStateSuite extends SparkFunSuite

  before {
    StreamingContext.getActive().foreach { _.stop(stopSparkContext = false) }


Just noticed. I think

foreach(_.stop(stopSparkContext = false))

might be better although this is not the part of this PR.

srowen · 2016-05-03T12:07:04Z

core/src/test/scala/org/apache/spark/scheduler/EventLoggingListenerSuite.scala

-            eventSet.remove(event)
+    try {
+      val logStart = SparkListenerLogStart(SPARK_VERSION)
+      val lines = readLines(logData)


Here, I think readLines is the only thing that reads logData? after it's done you could close the stream. Maybe that's tidier than wrapping so much in the try-finally block.

@srowen I tried:

val logData = EventLoggingListener.openEventLog(new Path(eventLogger.logPath), fileSystem) var lines : Seq[String] = null try { lines = readLines(logData) } finally { logData.close() }

It thrown IOException complaining that Stream closed.

I think the readLines is sort of lazy such that it won't read the file until we actually move the iterator.

OK sounds fine. How about pulling out eventSet at least?

andrewor14 · 2016-05-09T18:27:41Z

add to whitelist

SparkQA · 2016-05-09T18:38:05Z

Test build #58158 has finished for PR 12693 at commit 69dbceb.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

taoli91 · 2016-05-10T05:16:02Z

@andrewor14 I've fixed the failure. Could you please try it again? Thanks

SparkQA · 2016-05-10T06:52:16Z

Test build #58206 has finished for PR 12693 at commit 3ae2396.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

taoli91 · 2016-05-10T07:32:24Z

Well it's wired, I can't repro the test failure locally.

taoli91 · 2016-05-12T02:21:39Z

I can't repro the failure on my environment. It seems my changes on this commit shouldn't change the logic of the Launcher part. Is it possible that it's a random failure?

taoli91 · 2016-05-16T06:25:17Z

@andrewor14 Could you please schedule another test for me?

andrewor14 · 2016-05-16T18:13:17Z

retest this please

andrewor14 · 2016-05-16T18:13:26Z

you should be able to trigger it too

SparkQA · 2016-05-16T20:10:12Z

Test build #58652 has finished for PR 12693 at commit 3ae2396.

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

HyukjinKwon · 2016-09-21T01:28:45Z

@taoli91 Hi. Maybe we could close this and #12696 and then start to resolve https://issues.apache.org/jira/browse/SPARK-17591 first step by step? If we could verify the tests, I guess it'd be faster to merge each PR.

srowen · 2016-09-21T08:31:21Z

@taoli91 if you want to rebase this I'd be happy to finish the review too. It looked like it was almost there or already done.

HyukjinKwon · 2016-10-05T05:12:20Z

Oh, sorry, this was not Windows specific issue. Sorry for the comment above.

tdas · 2016-10-25T00:28:58Z

@HyukjinKwon could you rebase this PR. Thanks for the detailed work to find these issues. This is a very useful PR and would be good to merge if you rebase the PR.

HyukjinKwon · 2016-10-25T00:35:44Z

Sure, I definitely will.

Closes apache#11610 Closes apache#15411 Closes apache#15501 Closes apache#12613 Closes apache#12518 Closes apache#12026 Closes apache#15524 Closes apache#12693 Closes apache#12358 Closes apache#15588 Closes apache#15635 Closes apache#15678 Closes apache#14699 Closes apache#9008

taoli91 added 12 commits April 26, 2016 00:35

mvn.cmd for windows

1b531f8

Helping script for windows to download dependency and start zinc to support incremental building on windows.

Fix file not closed on FileSuite

aa31dd9

Fix ReliableCheckpointRDD file won't correctly closed if error on the…

151a1b0

… first stream

Fix another file closing

781b1ac

close log data.

5b83cf6

Close the class loader

aa5c099

Another file not closed.

6108f61

Stop to release resources.

8b98826

More closing problem

16edc2e

Fix the zip file and jar file in RPackageUtilsSuite

1f91a8d

Stop ssc in MasterFailureTest

e9399a3

Remove accidentally added files

0948fd2

srowen reviewed Apr 26, 2016
View reviewed changes

HyukjinKwon reviewed Apr 26, 2016
View reviewed changes

Code cleanup with respect to comments

488cd3a

srowen reviewed May 3, 2016
View reviewed changes

taoli91 added 2 commits May 4, 2016 20:51

Style fixes

aa74679

Minor code cleanup

69dbceb

Clear build warning

3ae2396

HyukjinKwon mentioned this pull request Oct 25, 2016

[SPARK-14914][CORE] Fix Resource not closed after using, mostly for unit tests #15618

Closed

srowen mentioned this pull request Oct 30, 2016

[BUILD] Close stale Pull Requests. #15685

Closed

asfgit closed this in 26b07f1 Oct 31, 2016

		@@ -39,18 +39,15 @@ class MapWithStateSuite extends SparkFunSuite

		before {
		StreamingContext.getActive().foreach { _.stop(stopSparkContext = false) }

[SPARK-14914] Fix Resource not closed after using, mostly for unit tests #12693

[SPARK-14914] Fix Resource not closed after using, mostly for unit tests #12693

Uh oh!

Conversation

taoli91 commented Apr 26, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

srowen Apr 26, 2016

Choose a reason for hiding this comment

Uh oh!

taoli91 Apr 26, 2016 • 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 Apr 26, 2016

Choose a reason for hiding this comment

Uh oh!

taoli91 May 3, 2016

Choose a reason for hiding this comment

Uh oh!

srowen May 3, 2016

Choose a reason for hiding this comment

Uh oh!

taoli91 May 4, 2016 • 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 May 4, 2016 • 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 Apr 26, 2016

Uh oh!

HyukjinKwon Apr 26, 2016

Choose a reason for hiding this comment

Uh oh!

srowen May 3, 2016

Choose a reason for hiding this comment

Uh oh!

taoli91 May 4, 2016 • 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 May 4, 2016

Choose a reason for hiding this comment

Uh oh!

andrewor14 commented May 9, 2016

Uh oh!

SparkQA commented May 9, 2016

Uh oh!

taoli91 commented May 10, 2016

Uh oh!

SparkQA commented May 10, 2016

Uh oh!

taoli91 commented May 10, 2016

Uh oh!

taoli91 commented May 12, 2016

Uh oh!

taoli91 commented May 16, 2016

Uh oh!

andrewor14 commented May 16, 2016

Uh oh!

andrewor14 commented May 16, 2016

Uh oh!

SparkQA commented May 16, 2016

Uh oh!

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

Uh oh!

Uh oh!

srowen commented Sep 21, 2016

Uh oh!

HyukjinKwon commented Oct 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tdas commented Oct 25, 2016

Uh oh!

HyukjinKwon commented Oct 25, 2016

Uh oh!

Uh oh!

taoli91 Apr 26, 2016 •

edited

Loading

taoli91 May 4, 2016 •

edited

Loading

srowen May 4, 2016 •

edited

Loading

taoli91 May 4, 2016 •

edited

Loading

HyukjinKwon commented Sep 21, 2016 •

edited

Loading

HyukjinKwon commented Oct 5, 2016 •

edited

Loading