attaching patch to get a QA run while I verify that this fixes the problem on the hbase side.

submitting patch -001 (had to temporarily assign to me to get into a state where I could do this)

knew I forgot something. thanks Steve!

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12704218/HADOOP-11710.1.patch.txt
against trunk revision b49c3a1.

+1 @author. The patch does not contain any @author tags.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5929//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5929//console

This message is automatically generated.

I'm attempting to manually test, but I've had tons of problems reproducing the failure reliably on my own machines.

Thanks, Sean. I think unit testing is gonna be tough because this is adding synchronization. By their nature, synchronization bugs tend to show up very intermittently. Since this is just adding synchronized to 3 functions I would say +1 as-is. I will give others a bit to comment before I commit

we don't have any formal FS concurrency tests. The load generator does this, but it's unlikely to be doing the operation replicating this problem.

Looking at the code, it seems OK to me, so +1

However, I would recommend one change, which is in close(), move the close=true operation up immediately after the close check, just in case something in {{freeBuffers() }} raised an exception or the parent did -it'll stop a second close() call getting into a mess. This is not really related to the rest of the patch, except in the general "improve re-entrancy" context

updated with feedback.

I have tested the v2 patch with my unit test that repros the original issue. +1 (non-binding).
I can post the new unit test under a separate JIRA.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12704284/HADOOP-11710.2.patch.txt
against trunk revision 8212877.

+1 @author. The patch does not contain any @author tags.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common:

org.apache.hadoop.crypto.TestCryptoStreamsForLocalFS
org.apache.hadoop.crypto.TestCryptoStreamsWithOpensslAesCtrCryptoCodec
org.apache.hadoop.crypto.TestCryptoStreams
org.apache.hadoop.crypto.TestCryptoStreamsNormal

Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5930//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5930//console

This message is automatically generated.

Those look related.

Will take a look in a couple hours or so.

–
Sean

Sean, don't move closed = true;.
super.close(); will invoke flush to flush the remaining data in the buffer, if we set closed to true before invoking super.close(), we will get error.
I think the test failure should be related to this.

Oh right, of course! Yes that's definitely the cause of the errors.

Patch vers 1 should be used then, unless someone feels strongly about the
placement wrt disposing of the direct buffers.

Oh, I just see Steve's comments

However, I would recommend one change, which is in close(), move the close=true operation up immediately after the close check, just in case something in {{freeBuffers() }} raised an exception or the parent did -it'll stop a second close() call getting into a mess. This is not really related to the rest of the patch, except in the general "improve re-entrancy" contex

I agree we should make closed be set to false, also I think use sun's API to release directbuffer rarely failed. Maybe we can put it in try... finally.

patch v3, uses try ... finally to always set closed on first call to close().

locally passed all the cypto tests:

-------------------------------------------------------
 T E S T S
-------------------------------------------------------
Running org.apache.hadoop.crypto.key.TestKeyProviderCryptoExtension
Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.753 sec - in org.apache.hadoop.crypto.key.TestKeyProviderCryptoExtension
Running org.apache.hadoop.crypto.TestCryptoCodec
Tests run: 3, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 11.974 sec - in org.apache.hadoop.crypto.TestCryptoCodec
Running org.apache.hadoop.crypto.TestCryptoStreams
Tests run: 14, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 29.953 sec - in org.apache.hadoop.crypto.TestCryptoStreams
Running org.apache.hadoop.crypto.TestCryptoStreamsForLocalFS
Tests run: 14, Failures: 0, Errors: 0, Skipped: 5, Time elapsed: 15.692 sec - in org.apache.hadoop.crypto.TestCryptoStreamsForLocalFS
Running org.apache.hadoop.crypto.TestCryptoStreamsNormal
Tests run: 14, Failures: 0, Errors: 0, Skipped: 8, Time elapsed: 6.46 sec - in org.apache.hadoop.crypto.TestCryptoStreamsNormal
Running org.apache.hadoop.crypto.TestCryptoStreamsWithOpensslAesCtrCryptoCodec
Tests run: 14, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 27.867 sec - in org.apache.hadoop.crypto.TestCryptoStreamsWithOpensslAesCtrCryptoCodec

Results :

Tests run: 61, Failures: 0, Errors: 0, Skipped: 15




-------------------------------------------------------
 T E S T S
-------------------------------------------------------
Running org.apache.hadoop.cli.TestCryptoAdminCLI
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 9.867 sec - in org.apache.hadoop.cli.TestCryptoAdminCLI
Running org.apache.hadoop.hdfs.crypto.TestHdfsCryptoStreams
Tests run: 14, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 37.574 sec - in org.apache.hadoop.hdfs.crypto.TestHdfsCryptoStreams

Results :

Tests run: 15, Failures: 0, Errors: 0, Skipped: 0

+1 pending Jenkins.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12704332/HADOOP-11710.3.patch.txt
against trunk revision 8212877.

+1 @author. The patch does not contain any @author tags.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common:

org.apache.hadoop.ipc.TestRPCWaitForProxy

Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5932//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5932//console

This message is automatically generated.

Committed to trunk and branch-2.

The test failure is not related.

FAILURE: Integrated in Hadoop-trunk-Commit #7314 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7314/)
HADOOP-11710. Make CryptoOutputStream behave like DFSOutputStream wrt synchronization. (Sean Busbey via yliu) (yliu: rev a85291003cf3e3fd79b6addcf59d4f43dc72d356)

• hadoop-common-project/hadoop-common/CHANGES.txt
• hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/CryptoOutputStream.java

FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #131 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/131/)
HADOOP-11710. Make CryptoOutputStream behave like DFSOutputStream wrt synchronization. (Sean Busbey via yliu) (yliu: rev a85291003cf3e3fd79b6addcf59d4f43dc72d356)

• hadoop-common-project/hadoop-common/CHANGES.txt
• hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/CryptoOutputStream.java

SUCCESS: Integrated in Hadoop-Yarn-trunk #865 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/865/)
HADOOP-11710. Make CryptoOutputStream behave like DFSOutputStream wrt synchronization. (Sean Busbey via yliu) (yliu: rev a85291003cf3e3fd79b6addcf59d4f43dc72d356)

• hadoop-common-project/hadoop-common/CHANGES.txt
• hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/CryptoOutputStream.java

FAILURE: Integrated in Hadoop-Mapreduce-trunk #2081 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2081/)
HADOOP-11710. Make CryptoOutputStream behave like DFSOutputStream wrt synchronization. (Sean Busbey via yliu) (yliu: rev a85291003cf3e3fd79b6addcf59d4f43dc72d356)

• hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/CryptoOutputStream.java
• hadoop-common-project/hadoop-common/CHANGES.txt

FAILURE: Integrated in Hadoop-Hdfs-trunk #2063 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2063/)
HADOOP-11710. Make CryptoOutputStream behave like DFSOutputStream wrt synchronization. (Sean Busbey via yliu) (yliu: rev a85291003cf3e3fd79b6addcf59d4f43dc72d356)

• hadoop-common-project/hadoop-common/CHANGES.txt
• hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/CryptoOutputStream.java

FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #122 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/122/)
HADOOP-11710. Make CryptoOutputStream behave like DFSOutputStream wrt synchronization. (Sean Busbey via yliu) (yliu: rev a85291003cf3e3fd79b6addcf59d4f43dc72d356)

• hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/CryptoOutputStream.java
• hadoop-common-project/hadoop-common/CHANGES.txt

FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #131 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/131/)
HADOOP-11710. Make CryptoOutputStream behave like DFSOutputStream wrt synchronization. (Sean Busbey via yliu) (yliu: rev a85291003cf3e3fd79b6addcf59d4f43dc72d356)

• hadoop-common-project/hadoop-common/CHANGES.txt
• hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/CryptoOutputStream.java

Yi Liu, I cherry-picked this to branch-2.7 since it's not committed to the branch and you targeted this to the branch. Feel free to revert it if you find any problem.

I cherry-picked this to branch-2.7

Oh, I missed that. Thanks for committing to branch-2.7, Tsuyoshi Ozawa.

Pulled this into 2.6.1 after Akira Ajisaka verified that the patch applies cleanly. Ran compilation before the push.

FAILURE: Integrated in HBase-TRUNK #6771 (See https://builds.apache.org/job/HBase-TRUNK/6771/)
HBASE-13221 List Hadoop 2.6 as unsupported due to HADOOP-11710. (busbey: rev e54cb8abebb2971da68852c6dcef456dbc689afb)

• src/main/asciidoc/_chapters/configuration.adoc