[SPARK-7660] Wrap SnappyOutputStream to work around snappy-java bug #6176

JoshRosen · 2015-05-15T06:48:01Z

This patch wraps SnappyOutputStream to ensure that close() is idempotent and to guard against write-after-close() bugs. This is a workaround for xerial/snappy-java#107, a bug where a non-idempotent close() method can lead to stream corruption. We can remove this workaround if we upgrade to a snappy-java version that contains my fix for this bug, but in the meantime this patch offers a backportable Spark fix.

JoshRosen · 2015-05-15T06:48:10Z

/cc @rxin @pwendell

AmplabJenkins · 2015-05-15T06:52:12Z

Merged build triggered.

AmplabJenkins · 2015-05-15T06:52:20Z

Merged build started.

pwendell · 2015-05-15T06:56:46Z

@JoshRosen does it make sense to re-enable those flakey tests in this patch?

rxin · 2015-05-15T06:58:42Z

core/src/main/scala/org/apache/spark/io/CompressionCodec.scala

+  private[this] var closed: Boolean = false
+
+  override def write(b: Int): Unit = {
+    if (closed) {


what are these guarding against? I understand you'd need it in close, but here?

In the unlikely event that we wrote to a closed stream, this would end up mutating a buffer that might be shared.

Note that the upstream fix also incorporates this: xerial/snappy-java#108 (comment)

AmplabJenkins · 2015-05-15T07:07:23Z

Merged build finished. Test FAILed.

AmplabJenkins · 2015-05-15T07:07:24Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/32789/
Test FAILed.

JoshRosen · 2015-05-15T07:14:18Z

Jenkins, retest this please.

JoshRosen · 2015-05-15T07:15:22Z

@pwendell, my hotfix didn't disable the tests; instead, it added a workaround in another test suite that clears the pool of reusable Snappy buffers, fixing the JavaAPISuite failures (I tested this locally before pushing the hotfix).

AmplabJenkins · 2015-05-15T07:17:10Z

Merged build triggered.

AmplabJenkins · 2015-05-15T07:17:20Z

Merged build started.

SparkQA · 2015-05-15T07:17:45Z

Test build #32793 has started for PR 6176 at commit 8b77aae.

SparkQA · 2015-05-15T09:05:10Z

Test build #32793 has finished for PR 6176 at commit 8b77aae.

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

AmplabJenkins · 2015-05-15T09:05:15Z

Merged build finished. Test PASSed.

AmplabJenkins · 2015-05-15T09:05:15Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/32793/
Test PASSed.

pwendell · 2015-05-15T16:57:44Z

@JoshRosen ah totally understand - got it.

pwendell · 2015-05-15T23:34:20Z

core/src/main/scala/org/apache/spark/io/CompressionCodec.scala

+ */
+private final class SnappyOutputStreamWrapper(os: SnappyOutputStream) extends OutputStream {
+
+  private[this] var closed: Boolean = false


Just double checking - this doesn't need to be volatile?

Ah, good point. I guess this needs to be volatile in case we're performing cleanup in another thread. @rxin, if this is volatile, won't that make the write() checks way more expensive?

I think we already do per-record checking of volatile vars.

https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/TaskContextImpl.scala#L44

On Fri, May 15, 2015 at 4:40 PM, Josh Rosen notifications@github.com
wrote:

In core/src/main/scala/org/apache/spark/io/CompressionCodec.scala
#6176 (comment):

}

override def compressedInputStream(s: InputStream): InputStream = new SnappyInputStream(s)
}
+
+/**

* Wrapper over [[SnappyOutputStream]] which guards against write-after-close and double-close

* issues. See SPARK-7660 for more details. This wrapping can be removed if we upgrade to a version

* of snappy-java that contains the fix for SnappyOutputStream.close() is not idempotent xerial/snappy-java#107.

*/
+private final class SnappyOutputStreamWrapper(os: SnappyOutputStream) extends OutputStream {
+

private[this] var closed: Boolean = false

Ah, good point. I guess this needs to be volatile in case we're performing
cleanup in another thread. @rxin https://github.com/rxin, if this is
volatile, won't that make the write() checks way more expensive?

—
Reply to this email directly or view it on GitHub
https://github.com/apache/spark/pull/6176/files#r30454371.

We discussed this offline: as far as we know, this is only ever executed in a single-threaded context (since tasks are single-threaded and their stop / cleanup logic is performed in that same thread), so the volatile should not be necessary.

pwendell · 2015-05-15T23:34:38Z

Josh this LGTM - if there are no outstanding comments we should try to get this in since it's one of the few remaining blockers.

rxin · 2015-05-17T00:50:39Z

LGTM.

JoshRosen · 2015-05-17T16:30:21Z

I'm going to merge this into master and branch-1.4, then will cherry-pick back to 1.3.x and 1.2.x. Thanks for the reviews.

This patch wraps `SnappyOutputStream` to ensure that `close()` is idempotent and to guard against write-after-`close()` bugs. This is a workaround for xerial/snappy-java#107, a bug where a non-idempotent `close()` method can lead to stream corruption. We can remove this workaround if we upgrade to a snappy-java version that contains my fix for this bug, but in the meantime this patch offers a backportable Spark fix. Author: Josh Rosen <joshrosen@databricks.com> Closes #6176 from JoshRosen/SPARK-7660-wrap-snappy and squashes the following commits: 8b77aae [Josh Rosen] Wrap SnappyOutputStream to fix SPARK-7660 (cherry picked from commit f2cc6b5) Signed-off-by: Josh Rosen <joshrosen@databricks.com>

This patch wraps `SnappyOutputStream` to ensure that `close()` is idempotent and to guard against write-after-`close()` bugs. This is a workaround for xerial/snappy-java#107, a bug where a non-idempotent `close()` method can lead to stream corruption. We can remove this workaround if we upgrade to a snappy-java version that contains my fix for this bug, but in the meantime this patch offers a backportable Spark fix. Author: Josh Rosen <joshrosen@databricks.com> Closes #6176 from JoshRosen/SPARK-7660-wrap-snappy and squashes the following commits: 8b77aae [Josh Rosen] Wrap SnappyOutputStream to fix SPARK-7660 (cherry picked from commit f2cc6b5) Signed-off-by: Josh Rosen <joshrosen@databricks.com> Conflicts: core/src/main/scala/org/apache/spark/io/CompressionCodec.scala core/src/test/java/org/apache/spark/shuffle/unsafe/UnsafeShuffleWriterSuite.java

This patch wraps `SnappyOutputStream` to ensure that `close()` is idempotent and to guard against write-after-`close()` bugs. This is a workaround for xerial/snappy-java#107, a bug where a non-idempotent `close()` method can lead to stream corruption. We can remove this workaround if we upgrade to a snappy-java version that contains my fix for this bug, but in the meantime this patch offers a backportable Spark fix. Author: Josh Rosen <joshrosen@databricks.com> Closes apache#6176 from JoshRosen/SPARK-7660-wrap-snappy and squashes the following commits: 8b77aae [Josh Rosen] Wrap SnappyOutputStream to fix SPARK-7660

Wrap SnappyOutputStream to fix SPARK-7660

8b77aae

rxin reviewed May 15, 2015
View reviewed changes

pwendell reviewed May 15, 2015
View reviewed changes

asfgit closed this in f2cc6b5 May 17, 2015

JoshRosen deleted the SPARK-7660-wrap-snappy branch May 26, 2015 03:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-7660] Wrap SnappyOutputStream to work around snappy-java bug #6176

[SPARK-7660] Wrap SnappyOutputStream to work around snappy-java bug #6176

JoshRosen commented May 15, 2015

JoshRosen commented May 15, 2015

AmplabJenkins commented May 15, 2015

AmplabJenkins commented May 15, 2015

pwendell commented May 15, 2015

rxin May 15, 2015

JoshRosen May 15, 2015

JoshRosen May 15, 2015

AmplabJenkins commented May 15, 2015

AmplabJenkins commented May 15, 2015

JoshRosen commented May 15, 2015

JoshRosen commented May 15, 2015

AmplabJenkins commented May 15, 2015

AmplabJenkins commented May 15, 2015

SparkQA commented May 15, 2015

SparkQA commented May 15, 2015

AmplabJenkins commented May 15, 2015

AmplabJenkins commented May 15, 2015

pwendell commented May 15, 2015

pwendell May 15, 2015

JoshRosen May 15, 2015

pwendell May 15, 2015

JoshRosen May 17, 2015

pwendell commented May 15, 2015

rxin commented May 17, 2015

JoshRosen commented May 17, 2015

[SPARK-7660] Wrap SnappyOutputStream to work around snappy-java bug #6176

[SPARK-7660] Wrap SnappyOutputStream to work around snappy-java bug #6176

Conversation

JoshRosen commented May 15, 2015

JoshRosen commented May 15, 2015

AmplabJenkins commented May 15, 2015

AmplabJenkins commented May 15, 2015

pwendell commented May 15, 2015

rxin May 15, 2015

Choose a reason for hiding this comment

JoshRosen May 15, 2015

Choose a reason for hiding this comment

JoshRosen May 15, 2015

Choose a reason for hiding this comment

AmplabJenkins commented May 15, 2015

AmplabJenkins commented May 15, 2015

JoshRosen commented May 15, 2015

JoshRosen commented May 15, 2015

AmplabJenkins commented May 15, 2015

AmplabJenkins commented May 15, 2015

SparkQA commented May 15, 2015

SparkQA commented May 15, 2015

AmplabJenkins commented May 15, 2015

AmplabJenkins commented May 15, 2015

pwendell commented May 15, 2015

pwendell May 15, 2015

Choose a reason for hiding this comment

JoshRosen May 15, 2015

Choose a reason for hiding this comment

pwendell May 15, 2015

Choose a reason for hiding this comment

JoshRosen May 17, 2015

Choose a reason for hiding this comment

pwendell commented May 15, 2015

rxin commented May 17, 2015

JoshRosen commented May 17, 2015