Skip to content

Commit fa4fbf9

Browse files
committed
Fix ConvictionPolicy.openConnections counter can go negative
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 cd0b08e commit fa4fbf9

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)