You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by JoshRosen <gi...@git.apache.org> on 2015/11/04 01:53:02 UTC
[GitHub] spark pull request: [SPARK-11495] Fix potential socket / file hand...
GitHub user JoshRosen opened a pull request:
https://github.com/apache/spark/pull/9455
[SPARK-11495] Fix potential socket / file handle leaks that were found via static analysis
The HP Fortify Opens Source Review team (https://www.hpfod.com/open-source-review-project) reported a handful of potential resource leaks that were discovered using their static analysis tool. We should fix the issues identified by their scan.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/JoshRosen/spark fix-potential-resource-leaks
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/9455.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #9455
----
commit 279eb246baa73ac6397852e4935d41ef19eabbfc
Author: Josh Rosen <jo...@databricks.com>
Date: 2015-11-03T23:46:21Z
Fix possible leak in JavaCustomReceiver.
commit a632cd8463030187a70ca9590ea630e05cb127f2
Author: Josh Rosen <jo...@databricks.com>
Date: 2015-11-03T23:53:26Z
Fix potential leak in JavaReceiverAPISuite
commit 1c0433d189192308711c1c2ad4f4ebce2242c3d0
Author: Josh Rosen <jo...@databricks.com>
Date: 2015-11-03T23:55:06Z
Release bufferChunk in ChunkFetchIntegrationSuite
commit cafdea94fd8d215964be27631af41e1e27d2bc89
Author: Josh Rosen <jo...@databricks.com>
Date: 2015-11-04T00:30:28Z
Fix potential file stream leak in UnsafeSorterSpillReader.
commit 4119fa9a73520e1451843ce1a6af8749aa6c82d9
Author: Josh Rosen <jo...@databricks.com>
Date: 2015-11-04T00:33:13Z
Address potential leaks in TestShuffleDataContext.
commit ab3478124bea8cddeb1dd35611ae0f5cf4a67588
Author: Josh Rosen <jo...@databricks.com>
Date: 2015-11-04T00:35:23Z
Fix potential resource leak in ChunkFetchIntegrationSuite
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-11495] Fix potential socket / file hand...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/9455#issuecomment-153537697
**[Test build #44984 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/44984/consoleFull)** for PR 9455 at commit [`ab34781`](https://github.com/apache/spark/commit/ab3478124bea8cddeb1dd35611ae0f5cf4a67588).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-11495] Fix potential socket / file hand...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/9455#issuecomment-153538647
**[Test build #44984 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/44984/consoleFull)** for PR 9455 at commit [`ab34781`](https://github.com/apache/spark/commit/ab3478124bea8cddeb1dd35611ae0f5cf4a67588).
* This patch **fails to build**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:\n * `public final class UnsafeSorterSpillReader extends UnsafeSorterIterator implements Closeable `\n * `case class ExecutorLostFailure(`\n * ` case class DecimalLit(chars: String) extends Token `\n
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-11495] Fix potential socket / file hand...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9455#issuecomment-153538652
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/44984/
Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-11495] Fix potential socket / file hand...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9455#issuecomment-153537164
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-11495] Fix potential socket / file hand...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9455#issuecomment-153537143
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-11495] Fix potential socket / file hand...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/9455#discussion_r43830137
--- Diff: core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeSorterSpillReader.java ---
@@ -31,10 +30,8 @@
* Reads spill files written by {@link UnsafeSorterSpillWriter} (see that class for a description
* of the file format).
*/
-public final class UnsafeSorterSpillReader extends UnsafeSorterIterator {
- private static final Logger logger = LoggerFactory.getLogger(UnsafeSorterSpillReader.class);
+public final class UnsafeSorterSpillReader extends UnsafeSorterIterator implements Closeable {
--- End diff --
Of the potential leaks that were discovered, I think that this is the only one to be concerned about: it looks like `UnsafeSorterSpillReader` might leak an open `FileInputStream` if an exception occurred while reading records. The fix implemented here is to add a `close()` method for safely closing the reader's streams.
I chose to move the spill deletion logic out of the reader itself since it appears to be redundant with spill deletion code that lives elsewhere. We should audit the existing code to make sure that the chain of responsibility for cleaning up spill files is clearly defined.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-11495] Fix potential socket / file hand...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9455#issuecomment-153538651
Merged build finished. Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-11495] Fix potential socket / file hand...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/9455#discussion_r43854421
--- Diff: core/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java ---
@@ -325,6 +327,11 @@ public Location next() {
try {
reader.loadNext();
} catch (IOException e) {
+ try {
+ reader.close();
+ } catch(IOException e2) {
+ logger.error("Error while closing spill reader", e2);
--- End diff --
Why are you not using ```Closables``` here? Custom error message?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org