Fix conviction policy counter go negative

[dkropachev]

DefaultConvictionPolicy.openConnections can become negative:

java.lang.AssertionError
at com.datastax.driver.core.ConvictionPolicy$DefaultConvictionPolicy.signalConnectionClosed(ConvictionPolicy.java:90)
at com.datastax.driver.core.Connection.closeAsync(Connection.java:953)
at com.datastax.driver.core.HostConnectionPool.discardAvailableConnections(HostConnectionPool.java:882)
at com.datastax.driver.core.HostConnectionPool.closeAsync(HostConnectionPool.java:843)

It happens because there's a loose connection between Connection and ConvictionPolicy (i.e., Host), which makes it impossible to guarantee that a single connection always address same openConnections counter.

This can occasionally lead to assertion failures in rare cases on given line:
assert remaining >= 0;

Fixing this would require refactoring Connection.java to bind connections to Host instead of Endpoint, which is not justified considering low impact of this issue.

Fixes: #82

[fruch]

nice warpup, of you fixing the issue you raise almost 3 years ago :)

[Bouncheck]

List<Connection> newConnections(HostConnectionPool pool, int count) calls List<Connection> connections = Lists.newArrayListWithCapacity(count); which should throw IllegalArgumentException when the count is negative. While it's better to guard signal against negatives which technically could still happen, was that exception ever seen? It would be weird if not.

[dkropachev]

List<Connection> newConnections(HostConnectionPool pool, int count) calls List<Connection> connections = Lists.newArrayListWithCapacity(count); which should throw IllegalArgumentException when the count is negative. While it's better to guard signal against negatives which technically could still happen, was that exception ever seen? It would be weird if not.

It is 0 at the time, reduced by 1 afterwards.

[Bouncheck]

List<Connection> newConnections(HostConnectionPool pool, int count) calls List<Connection> connections = Lists.newArrayListWithCapacity(count); which should throw IllegalArgumentException when the count is negative. While it's better to guard signal against negatives which technically could still happen, was that exception ever seen? It would be weird if not.

It is 0 at the time, reduced by 1 afterwards.

I don't follow. The count argument in newConnections? openConnections after signalConnectionsOpening?
What about the exception?

[Bouncheck]

unblocking

[dkropachev]

List<Connection> newConnections(HostConnectionPool pool, int count) calls List<Connection> connections = Lists.newArrayListWithCapacity(count); which should throw IllegalArgumentException when the count is negative. While it's better to guard signal against negatives which technically could still happen, was that exception ever seen? It would be weird if not.

It is 0 at the time, reduced by 1 afterwards.

I don't follow. The count argument in newConnections? openConnections after signalConnectionsOpening? What about the exception?

Yes, you are correct, let me look into it furthere.

[dkropachev]

@Bouncheck , please check again, I have decided to remove assert, reasoning on the PR description, commit and comments.

[Bouncheck]

IIUC assert removal is necessary. I think removing assert line and adding a comment is enough.
I see no reason to increment the counter when going negative. It does not fix the behavior and with this increment the number loses its meaning. Even if it was negative it would at least always be equal to <opens performed - closes performed>.

[dkropachev]

IIUC assert removal is necessary. I think removing assert line and adding a comment is enough. I see no reason to increment the counter when going negative. It does not fix the behavior and with this increment the number loses its meaning. Even if it was negative it would at least always be equal to <opens performed - closes performed>.

You are correct, but we have to fix signalConnectionFailure then to return true when counter less then 0 too, please take look.