[SPARK-14914] Fix Resource not closed after using, mostly for unit tests by taoli91 · Pull Request #12693 · apache/spark

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

 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:
vardeserializeStream:DeserializationStream=nullvalpartitioner=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

@@ -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

- 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:
vallogData=EventLoggingListener.openEventLog(newPath(eventLogger.logPath), fileSystem) varlines:Seq[String] =nulltry { 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.

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

Closesapache#11610Closesapache#15411Closesapache#15501Closesapache#12613Closesapache#12518Closesapache#12026Closesapache#15524Closesapache#12693Closesapache#12358Closesapache#15588Closesapache#15635Closesapache#15678Closesapache#14699Closesapache#9008 Author: Sean Owen <sowen@cloudera.com> Closesapache#15685 from srowen/CloseStalePRs.

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 26b07f1Oct 31, 2016

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

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

Uh oh!

Conversation

taoli91 commented Apr 26, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

srowenApr 26, 2016

Choose a reason for hiding this comment

Uh oh!

taoli91Apr 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!

srowenApr 26, 2016

Choose a reason for hiding this comment

Uh oh!

taoli91May 3, 2016

Choose a reason for hiding this comment

Uh oh!

srowenMay 3, 2016

Choose a reason for hiding this comment

Uh oh!

taoli91May 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!

srowenMay 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!

HyukjinKwonApr 26, 2016

Choose a reason for hiding this comment

Uh oh!

srowenMay 3, 2016

Choose a reason for hiding this comment

Uh oh!

taoli91May 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!

srowenMay 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!

Reviewers

Assignees

Labels

taoli91Apr 26, 2016•
edited
Loading

taoli91May 4, 2016•
edited
Loading

srowenMay 4, 2016•
edited
Loading

taoli91May 4, 2016•
edited
Loading

HyukjinKwon commented Sep 21, 2016•
edited
Loading

HyukjinKwon commented Oct 5, 2016•
edited
Loading