Details
-
Bug
-
Status: Closed
-
Minor
-
Resolution: Abandoned
-
None
-
None
-
None
Description
Currently, the incr/decr operation on the ref count of HConnectionImplementation are not atomic. This may cause that the ref count always be larger than 0 and the connection never be closed.
/** * Increment this client's reference count. */ void incCount() { ++refCount; } /** * Decrement this client's reference count. */ void decCount() { if (refCount > 0) { --refCount; } }
Attachments
Attachments
- HBASE-11685-trunk-v6.diff
- 2 kB
- Shaohui Liu
- HBASE-11685-trunk-v5.diff
- 2 kB
- Shaohui Liu
- HBASE-11685-trunk-v4.diff
- 2 kB
- Shaohui Liu
- HBASE-11685-trunk-v3.diff
- 2 kB
- Shaohui Liu
- HBASE-11685-trunk-v2.diff
- 2 kB
- Shaohui Liu
- HBASE-11685-trunk-v1.diff
- 2 kB
- Shaohui Liu
Activity
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12660120/HBASE-11685-trunk-v1.diff
against trunk revision .
ATTACHMENT ID: 12660120
+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 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc. The javadoc tool did not generate any warning messages.
+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 lineLengths. The patch does not introduce lines longer than 100
+1 site. The mvn site goal succeeds with this patch.
-1 core tests. The patch failed these unit tests:
org.apache.hadoop.hbase.client.TestClientNoCluster
Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html
Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//console
This message is automatically generated.
This change does not preserve the old behavior where we avoid taking refCount negative. A concern? We should warn loudly if this goes negative at least I think.
/** * Decrement this client's reference count. */ void decCount() { - if (refCount > 0) { - --refCount; - } + refCount.decrementAndGet(); }
Add warn log when the ref count is nagative.
Thanks for apurtell's review.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12660302/HBASE-11685-trunk-v3.diff
against trunk revision .
ATTACHMENT ID: 12660302
+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 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc. The javadoc tool did not generate any warning messages.
+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 lineLengths. The patch does not introduce lines longer than 100
+1 site. The mvn site goal succeeds with this patch.
-1 core tests. The patch failed these unit tests:
org.apache.hadoop.hbase.util.TestHBaseFsck
org.apache.hadoop.hbase.master.TestMasterOperationsForRegionReplicas
-1 core zombie tests. There are 3 zombie test(s):
Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html
Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//console
This message is automatically generated.
There's a spelling error in the log message. Please change "nagative" to "negative". Is there any way to identify for which connection the ref count went negative? Would aid in debugging. Otherwise lgtm.
Fix the typo and add connection name in warn log for debug.
Thanks for apurtell's suggestion.
Sorry to keep coming back to this.
*/ boolean isZeroReference() { - return refCount == 0; + return refCount.get() == 0; }
We use isZeroReference to detect if a connection can be reaped. If the count goes negative, and it can now after these changes, then isZeroReference will return false and the connection will never be closed out. I really think we need to preserve the earlier logic. This change seems likely to introduce a new class of bug
I really think we need to preserve the earlier logic
I'm sorry that i didn't get your meanings? is the earlier logic just the current codebase?
The current codebase, the connection may never be closed out for two threads concurrently decrease the ref counter.
And the following code just hides the the possible bug that there are more decCount ops than incCount ops in the code.
void decCount() {
if (refCount > 0) {
--refCount;
}
}
In my opinion,we should expose the bug as early as possible.
(1) When the count goes negative, we should print warn log or throw an exception.
(2) When someone forgets to call the decCount, and the ref count always be positive , the connection never be closed. we can find this problem from the output of jstack.
I think the change doesn't introduce a new class of bug.
> is the earlier logic just the current codebase?
What I mean by that is this:
void decCount() {
if (refCount > 0) {
--refCount;
}
}
This code does not allow the reference count to go below 0, which is what isZeroReference expects. The latest patch allows the reference count to go below 0 without changing isZeroReference or its callers to handle cases where the refcount is negative.
> When the count goes negative, we should print warn log or throw an exception.
Throwing an exception after insuring the connection is cleaned up is good, that would address the concern here.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12661133/HBASE-11685-trunk-v5.diff
against trunk revision .
ATTACHMENT ID: 12661133
+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 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc. The javadoc tool did not generate any warning messages.
+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 lineLengths. The patch does not introduce lines longer than 100
+1 site. The mvn site goal succeeds with this patch.
-1 core tests. The patch failed these unit tests:
org.apache.hadoop.hbase.TestMetaTableAccessorNoCluster
org.apache.hadoop.hbase.security.token.TestZKSecretWatcher
-1 core zombie tests. There are 1 zombie test(s):
Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//console
This message is automatically generated.
Fix the failed test TestMetaTableAccessorNoCluster.
And It seems that the other failed test TestZKSecretWatcher has no relations with this patch.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12661161/HBASE-11685-trunk-v6.diff
against trunk revision .
ATTACHMENT ID: 12661161
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 3 new or modified tests.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc. The javadoc tool did not generate any warning messages.
+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 lineLengths. The patch does not introduce lines longer than 100
+1 site. The mvn site goal succeeds with this patch.
-1 core tests. The patch failed these unit tests:
org.apache.hadoop.hbase.TestRegionRebalancing
-1 core zombie tests. There are 1 zombie test(s): at org.apache.hadoop.hbase.client.TestHCM.testClusterStatus(TestHCM.java:250)
Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html
Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//console
This message is automatically generated.
+ refCount = new AtomicInteger(1);
Could be replaced by a simple set, this would allow refCount to be final. It's theoretically better.
I wonder if the "throw new RuntimeException("Negative ref count of connection: " + this;" cannot have a race condition with this set to 1 in the finalize?
It could be simple to simply log a warning?
The problem with only logging a warning is we have code that expects the count never to go below zero and this patch is not changing callers of isZeroReference. Though it suppose isZeroReference could be updated here to return true if <= 0
Actually, there is no bug here: increments and decrements are done in synchronized (CONNECTION_INSTANCES) , so they are already atomic. There is no need to use atomicInteger here (a comment on the sync requirement would be welcome however )
The exception is the finalize (before and after the patch): there is a set without synchronization.
At the end, I would propose to just remove the finalize.. It's safer and makes the code simpler to read. We cannot rely on a finalize to free resources anyway.
The ref count is removed in HBase 2.0 (see HBASE-14787). Shall we close this jira? Or change the version to branch-1 ?
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 0s | Docker mode activated. |
0 | patch | 0m 6s | The patch file was not named according to hbase's naming conventions. Please see https://yetus.apache.org/documentation/0.3.0/precommit-patchnames for instructions. |
-1 | patch | 0m 10s | |
Subsystem | Report/Notes |
---|---|
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12661161/HBASE-11685-trunk-v6.diff |
JIRA Issue | |
Console output | https://builds.apache.org/job/PreCommit-HBASE-Build/6465/console |
Powered by | Apache Yetus 0.3.0 http://yetus.apache.org |
This message was automatically generated.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 0s | Docker mode activated. |
0 | patch | 0m 2s | The patch file was not named according to hbase's naming conventions. Please see https://yetus.apache.org/documentation/0.4.0/precommit-patchnames for instructions. |
-1 | patch | 0m 6s | |
Subsystem | Report/Notes |
---|---|
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12661161/HBASE-11685-trunk-v6.diff |
Console output | https://builds.apache.org/job/PreCommit-HBASE-Build/7431/console |
Powered by | Apache Yetus 0.4.0 http://yetus.apache.org |
This message was automatically generated.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 0s | Docker mode activated. |
0 | patch | 0m 3s | The patch file was not named according to hbase's naming conventions. Please see https://yetus.apache.org/documentation/0.8.0/precommit-patchnames for instructions. |
-1 | patch | 0m 5s | |
Subsystem | Report/Notes |
---|---|
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12661161/HBASE-11685-trunk-v6.diff |
Console output | https://builds.apache.org/job/PreCommit-HBASE-Build/15841/console |
Powered by | Apache Yetus 0.8.0 http://yetus.apache.org |
This message was automatically generated.
Change the refCount from int to AtomicInteger