Skip to content

Comprehensively retry poll/register even on unforeseen errors #563

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
gnprice opened this issue Mar 12, 2024 · 2 comments · Fixed by #1063
Closed

Comprehensively retry poll/register even on unforeseen errors #563

gnprice opened this issue Mar 12, 2024 · 2 comments · Fixed by #1063
Assignees
Labels
a-api Implementing specific parts of the Zulip server API

Comments

@gnprice
Copy link
Member

gnprice commented Mar 12, 2024

This is a followup to:

Hopefully with #556 (fixed by #561) we're done with needing to add try/catch blocks and retries for conditions that can happen for purely operational reasons like the device going to sleep. But there might be another such condition we've missed; and there's also always the possibility of bugs in our code. In particular the event-poll loop calls PerAccountStore.handleEvent, which can reach quite a lot of our model code because of the variety of different events, so it's probable that at some point we'll have a bug that causes that to throw an exception.

For the bulk of our code, if a bug causes an exception, then the best thing to do is to let it propagate out of our code uncaught — it'll mean that that widget doesn't get built (causing a gray rectangle in release builds), or that user gesture doesn't get acted on, but the user can pick up from there and try again. But the event loop, or UpdateMachine, is different: it's the one spot in our code where we have a persistent thread of control flow of our own. It needs to keep itself going no matter what happens, because nothing else will resume it for us.

I have a rough draft for this, which I wrote while working on #556/#561. I put it aside in order to get #561 out the door promptly, but I'll plan to pick it up later. The draft also comes with some user feedback, a form of #555.

Related issues, some of which might end up getting handled at the same time as this one:

@gnprice gnprice added the a-api Implementing specific parts of the Zulip server API label Mar 12, 2024
@gnprice gnprice added this to the Beta 2 milestone Mar 12, 2024
@gnprice gnprice self-assigned this Mar 12, 2024
@PIG208
Copy link
Member

PIG208 commented Aug 5, 2024

The referenced draft seems to be https://github.com/gnprice/zulip-flutter/tree/dev-retry. I can pick this up to work on #555.

@gnprice
Copy link
Member Author

gnprice commented Aug 16, 2024

This came up again at #809 (comment) (though that happened to be a false alarm). As I wrote there, restating the main substance of this issue but more briefly:

When we throw an exception from within PerAccountStore.handleEvent, that represents a bug but we can and should recover from it:

gnprice added a commit to gnprice/zulip-flutter that referenced this issue Nov 16, 2024
…ackoff

This fixes zulip#563.  We'll follow up with a few more commits that give
more-informative errors in some cases, or change the handling of others
from retrying getEvents to reloading the data from scratch.

The if-disposed and store.isLoading lines have no effect in the case
of a BAD_EVENT_QUEUE_ID error, because those can only come from the
getEvents request, and then the inner catch block will have already
taken the same steps.

Fixes: zulip#563
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-api Implementing specific parts of the Zulip server API
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants