Skip to content

Commit a4213a3

Browse files
committed
Fix ConvictionPolicy.openConnections counter can go negative (#466)
Loose connection between Connection and ConvictionPolicy (i.e Host) it is impossible to guarantee that single connection addresses same `openConnections` counter Which opens up opportunity for this assertion to fail in rare cases. Fixing this issue would involve major refactoring of `Connection.java` binding connection to `Host`, instead of `Endpoint` Giving low impact of given issue doing refactoring makes no sense.
1 parent 212acef commit a4213a3

File tree

1 file changed

+29
-5
lines changed

1 file changed

+29
-5
lines changed

driver-core/src/main/java/com/datastax/driver/core/ConvictionPolicy.java

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -87,8 +87,20 @@ void signalConnectionsOpening(int count) {
8787
@Override
8888
void signalConnectionClosed(Connection connection) {
8989
int remaining = openConnections.decrementAndGet();
90-
assert remaining >= 0;
91-
Host.statesLogger.debug("[{}] {} closed, remaining = {}", host, connection, remaining);
90+
// There's a loose connection between `Connection` and `ConvictionPolicy` (i.e., `Host`),
91+
// making it impossible to guarantee that a single connection always address same
92+
// `openConnections` counter.
93+
// This can occasionally lead to assertion failures in rare cases.
94+
// assert remaining >= 0;
95+
// Fixing this would require refactoring `Connection.java` to bind connections to `Host`
96+
// instead of `Endpoint`,
97+
// which is not justified considering low impact of this issue.
98+
if (remaining < 0) {
99+
Host.statesLogger.error(
100+
"[{}] {} closed, remaining is negative {}", host, connection, remaining);
101+
} else {
102+
Host.statesLogger.debug("[{}] {} closed, remaining = {}", host, connection, remaining);
103+
}
92104
}
93105

94106
@Override
@@ -98,12 +110,24 @@ boolean signalConnectionFailure(Connection connection, boolean decrement) {
98110
if (host.state != Host.State.DOWN) updateReconnectionTime();
99111

100112
remaining = openConnections.decrementAndGet();
101-
assert remaining >= 0;
102-
Host.statesLogger.debug("[{}] {} failed, remaining = {}", host, connection, remaining);
113+
// There's a loose connection between `Connection` and `ConvictionPolicy` (i.e., `Host`),
114+
// making it impossible to guarantee that a single connection always address same
115+
// `openConnections` counter.
116+
// This can occasionally lead to assertion failures in rare cases.
117+
// assert remaining >= 0;
118+
// Fixing this would require refactoring `Connection.java` to bind connections to `Host`
119+
// instead of `Endpoint`,
120+
// which is not justified considering low impact of this issue.
121+
if (remaining < 0) {
122+
Host.statesLogger.error(
123+
"[{}] {} failed, remaining is negative {}", host, connection, remaining);
124+
} else {
125+
Host.statesLogger.debug("[{}] {} failed, remaining = {}", host, connection, remaining);
126+
}
103127
} else {
104128
remaining = openConnections.get();
105129
}
106-
return remaining == 0;
130+
return remaining <= 0;
107131
}
108132

109133
private synchronized void updateReconnectionTime() {

0 commit comments

Comments
 (0)