It seems there were two mechanisms for assessing whether a CNode was still in use: a refcount and a release timestamp. The latter seems to have been there for a long time, as a safety mechanism.
However, this timer also keeps CNode objects alive for far longer than necessary after disconnects, potentially opening up a DoS window.
This commit removes the timestamp-based mechanism, and replaces it with an assert(nRefCount >= 0), to verify that the refcounting is indeed correctly working.
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/307465b04e408e67cca991cf11b8238a487ba01f for binaries and test log. This is an automated test script which runs test cases on each commit every time is updated. It, however, dies sometimes and fails to test properly, if you are waiting on a test, please check timestamps and if the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ and contact BlueMatt on freenode if something looks broken.
Rebased.
ACK
BTW maybe we could use RAII or even one of the boost reference counting pointers here, instead of explicit AddRef/Release, as that tends to be somewhat more robust.
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/670921ec410923fa70a38dd44fdfec157bc6f8ca for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
It seems there were two mechanisms for assessing whether a CNode
was still in use: a refcount and a release timestamp. The latter
seems to have been there for a long time, as a safety mechanism.
However, this timer also keeps CNode objects alive for far longer
than necessary after disconnects, potentially opening up a DoS
window.
This commit removes the timestamp-based mechanism, and replaces
it with an assert(nRefCount >= 0), to verify that the refcounting
is indeed correctly working.
Rebased.
ACK. This has survived several days of testing and abuse.
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/cedaa714462871213472019545b8e862dacdac91 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
I've been testing this with an extra commit that prints out vNodesDisconnected.size() at shutdown, and this seems to be always 0. That means that the refcounting is indeed working as intended: it doesn't go too low (tested by the assert), and not too high (as that would mean they linger in vNodesDisconnected until they reach 0).
ACK