Fix bug where kazoo client returns SESSION EXPIRED when in CONNECTING state

[prajay]

kazoo client returns a SessionExpired when its state is in CONNECTING. This is not an indication of session expiry and could just be that the client is still re-establishing a connection to the server after a network blip. Hence, the error returned should be a ConnectionLoss and not a SessionExpired

[prajay]

Would it be possible to restart the failed jobs on travis-ci? I can see that all jobs succeeded when I triggered a build on my fork

[StephenSorriaux]

Hi,

Thanks for this PR, I will link into it. Don't bother with the tests for now, I will re-trigger the Travis build.

[prajay]

Hey @StephenSorriaux,

Just wanted to check on this. Does the change seem reasonable to you?

[StephenSorriaux]

Hey @prajay,

I did not have time to go deep in the reason why we chose to raise SessionExpiredError in this case. Also, this change is not that straightforward since some users might be catching this exception for their use case, so ,as if, changing it to ConnectionLoss would be a breaking change.

[ztzg]

Hi @StephenSorriaux, @prajay,

A note to let you know that we are also seeing the issue addressed by this PR (also reported as
#374) and would like to see something akin to this patch in Kazoo master.

The main issue is that spurious "session expired" exceptions can cause applications to tear down much more state than required, and to try to rebuild it—further stressing an ensemble which may just have, for example, lost its leader.

I see that the goal of the commit (by @bbangert) which introduced the CONNECTING check was to allow applications to distinguish between a method invoked on a CLOSED client and a server-generated SESSIONEXPIRED (error code -112): 4eabbd1

I basically see three ways of allowing applications to discriminate between these different states while retaining "full" backward compatibility:

1. No code change, but a "redefinition" of "session expired" to SessionExpiredError + client.client_state == KeeperState.EXPIRED_SESSION.

2. Keeping the KeeperState, or alternatively, "fake codes" in the existing exception. Something akin to:

   @_zookeeper_exception(-112)
   class SessionExpiredError(ZookeeperError):
       def __init__(self, *, state=None):
           self.state = state

3. Taking inspiration of the commit above: introducing distinct subclasses of SessionExpiredError (one per actual condition) and redirecting EXCEPTIONS[-112] to one of them.

Each of these options is "distasteful" in its own way, but I could nevertheless whip up patches/PRs for #3 and/or #2 if you think they would be worth a closer look.

(#3 would probably be more "natural" if we manage to find "reasonable" subclass names. ConnectingError and ServerSessionExpiredError are the best I could come up with so far. Suggestions are welcome!)

[StephenSorriaux]

Hi @ztzg,

Thanks for taking some time to share your thoughts about this subject.

I understand what you want to do the and I start to think that the CONNECTING state should actually not raise any exception but, instead, just makes the request be queued as it is done with the CONNECTED state. Since the request stays in the queue as long as no connection is available, it should work fine but that definitely needs some testing (no side-effect on recipes, ensure that we don't keep too much requests, etc.).

[ztzg]

Not raising an exception on CONNECTING does indeed sound like the best solution (and matches what other clients are doing).

I had written it off as requiring invasive changes and/or going against the "philosophy" of Kazoo (no setWatches, etc.). I could have a closer look and see if I can come up with a proof of concept which is not too disruptive, if you think it has a chance to move forward.

[StephenSorriaux]

I really would like to more forward with this issue, so yes any contributions is very welcomed.

[jeffwidman]

Nudge @ztzg

[ztzg]

@jeffwidman: Still in my TODO list, but other things got on top. You're right that I should move forward a bit on this front, though. I'll keep your nudge in mind! :)

[StephenSorriaux]

Thanks @prajay for your time on this PR, I am closing it in favor of #588.