3rd-Eden/node-memcached — pull request #119 by ianshward (2013-07-17 22:12:18+00:00)
Title: this.retries in IssueLog reaches negative value

this.retries in the IssueLog object is able to reach a negative state, and when it does, a server is not removed when it should be. I believe the case where this happens is as follows:

- A memcached server stops working
- The memcached client has retries set to 3, which is used in Jackpot to [retry connection allocation](https://github.com/3rd-Eden/jackpot/blob/master/index.js#L116-L118)
- 5 simultaneous requests come in causing, for example, GET commands in node-memcached
- All 5 requests throw an error on the socket, ie, ECONNREFUSED, so connectionIssue is called for all 5 requests, which calls issues.log (IssueLog.log())
- In IssueLog.log(), it appears it's possible for all 5 requests to make it to [setTimeout(issue.attemptRetry.bind(issue), this.retry);](https://github.com/3rd-Eden/node-memcached/blob/master/lib/connection.js#L47), and all five timers will fire after this.retry, decrementing this.retries to -2.
- More errors occur, possible from Jackpot trying to reallocate connections for the original requests, which will make it to [here again](https://github.com/3rd-Eden/node-memcached/blob/master/lib/connection.js#L46) but this.retries will already be -2.

I guess the easiest fix would be to put a [lock around this check](https://github.com/3rd-Eden/node-memcached/blob/master/lib/connection.js#L46) and have the check be if (this.retries && !locked) and only allow one connection issue at a time to mark the server as failed.

I wrote up some notes on #116 about how this.retries works, but, at the time I did not see this race condition issue. I'll see how it looks/works if there's a lock in place.