api: Use exponential backoff in call_on_each_event.

[neiljp]

A long-term TODO here has been to move from a 1s pause after each failure towards an exponential backoff approach, as proposed here.

@timabbott See last_event_id handling topic on #zulip-terminal from last August.

I'm unsure of the best parameter set to use here; I've taken values from other uses of the backoff and filled the default values in explicitly to aid in reasoning.

I'm not sure if we want to switch to backoff.keep_going but rather stay with the previous while True approach, since this is essentially an unending event loop.

[timabbott]

Maybe we can go with maximum_retries=50, i.e. basically arranging it so that it will eventually give up, but only if it's really clear the server is down for good and not just having temporary downtime.

I also notice there's a time.sleep(1) in do_register; probably that should be changed as well.

(Or maybe do_register should be folded into this loop?)

[neiljp]

With this PR, the behavior is changed from before, which is a little concerning, though arguably useful - the handler will return after some time, rather than be in an infinite loop, so the caller can decide whether to retry again?

We could put the backoff parameters into the method call, or something like them; that could also enable behavior like previously (regarding the infinite loop), but with backoff?

[alexmv]

I think it is correct to potentially fail, so the caller knows something is horribly wrong. We have a bunch of clients that are forever looping and getting 401s during registration, and I don't think that their owners have any sign that anything is going wrong.

I think we should raise an exception if we hit the max retries, since existing code is likely not expecting the function to ever return. Raising an exception means it won't unexpectedly fall through to other code.

This behavior change certainly merits a documentation update as well.

[neiljp]

@timabbott I've added some followup commits which first do backoff for the do_register as well, and then inline it like you mentioned, which I think is cleaner. I'm not sure whether we want the intermediate refactoring or not.

There's also a 1s sleep in the error_retry in do_api_query, but I'm not sure if we want that to have backoff too?

[alexmv]

I don't think we want a timeout_success_equivalent on this. That's for cases where we don't have the ability to put an explicit success() call in, and we want to be able to reset our backoff after 5 minutes on the assumption that that's a "success." Here, we have places we can mark as successful. Having a timeout_success_equivalent muddies the logic -- and I think won't kick in unless we have 5-minute HTTP requests, which should really not be treated as successes. :)

[alexmv]

There's also a 1s sleep in the error_retry in do_api_query, but I'm not sure if we want that to have backoff too?

I think we certainly should. If we start to fall over due to overload and start returning 5xx, I don't think we want every client switching from 1 request every 10s (longpoll) to every second -- which is what the current logic does.

[timabbott]

@neiljp do you have time to update this? With #611 I'd like to get this resolve and then do a release.

[zulipbot]

Heads up @neiljp, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.