Skip to content

Commit b8fe3c6

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 b8fe3c6

File tree

1 file changed

+30
-4
lines changed

1 file changed

+30
-4
lines changed

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

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -87,8 +87,21 @@ 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+
openConnections.incrementAndGet();
102+
} else {
103+
Host.statesLogger.debug("[{}] {} closed, remaining = {}", host, connection, remaining);
104+
}
92105
}
93106

94107
@Override
@@ -98,8 +111,21 @@ boolean signalConnectionFailure(Connection connection, boolean decrement) {
98111
if (host.state != Host.State.DOWN) updateReconnectionTime();
99112

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

0 commit comments

Comments
 (0)