Skip to content
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

Retry failed "mark as read" requests #4559

Open
WesleyAC opened this issue Mar 22, 2021 · 4 comments
Open

Retry failed "mark as read" requests #4559

WesleyAC opened this issue Mar 22, 2021 · 4 comments
Labels
a-data-sync Zulip's event system, event queues, staleness/liveness P1 high-priority

Comments

@WesleyAC
Copy link
Contributor

WesleyAC commented Mar 22, 2021

(Update 2022-11: Since #4620, we do retry these requests. But only (a) the next time the user scrolls past some other unread messages, which may happen arbitrarily far in the future or never happen, and (b) while the app remains running -- we don't persist to disk the user's desire to mark the messages as read. So the main content of this issue still applies.)


Currently, if a API request to mark messages as read fails, we never retry it. Instead, we should save a list of messages we have tried to mark as read (this should be persisted to disk, ideally, although maybe just putting it in the Redux state is a good enough start), and retry them periodically (probably using exponential backoff, maybe with some jitter?)

I think we should probably do this for every messagesFlags call, not just read state: thing like starring messages deserve to not be lost as well.

One thing I'm not sure about is how to deal with stateful logic in the src/api/ code — I don't know what dependencies we're OK with that code having (can it save stuff in Redux?). I think it would be good to aim to have fairly generic code for retrying any sort of API request, which we can at first just have apply to messagesFlags, but can add to other methods later, if we want.

It's also worth thinking about how the auth will get passed around when we do this — we want whatever is clearing out the retry queue to have the most recent auth rather than the one that the message was first sent with, most likely. We should also make sure that the code doesn't accidentally send messages to the wrong realm.

Related: #4558, #4526, #3549, #3423, #500

@WesleyAC WesleyAC added the a-data-sync Zulip's event system, event queues, staleness/liveness label Mar 22, 2021
@chrisbobbe
Copy link
Contributor

chrisbobbe commented Mar 22, 2021

fairly generic code for retrying any sort of API request

I believe tryFetch might be helpful here?

I've also got a promiseTimeout in #4193 (not yet merged) that might be helpful if we want to give up retrying. promiseTimeout also came up in a fairly recent discussion on timeout/retry logic for sending outbox messages, here.

@gnprice
Copy link
Member

gnprice commented Mar 22, 2021

This will be good to do! Among other things it's a prerequisite to #3549.

Some discussion in chat about how to approach the implementation.

@gnprice
Copy link
Member

gnprice commented Apr 12, 2021

Among other things it's a prerequisite to #3549 [eagerly setting the flag locally before hearing back from the server].

Actually, thinking about this again I think it's not. (Though it'd still be good to do.) We can go ahead and do #3549 without blocking on this.

I explained why at an old comment #3549 (comment) :

For a reaction or vote or sent message, after the user has taken the action they may be counting on it having gone through -- on the other people in the conversation seeing what they did. So if it didn't go through after all, it can be important to let the user know that so they can follow up accordingly. Also, especially for a message, we don't want to retry indefinitely because after a certain point it could be worse to send the message late than not at all.

For a read flag, though, it's private to the user... and if it doesn't go through, the most that can cause is that they'll see the message again when catching up through some other client. In particular, failing to go through serves inherently as a way of notifying the user that it didn't go through.

@gnprice gnprice changed the title Retry failed "mark as read" messages Retry failed "mark as read" requests Apr 15, 2021
@WesleyAC
Copy link
Contributor Author

Started a conversation on CZO #mobile-team > non-redux storage? about using SQLite to persist this data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-data-sync Zulip's event system, event queues, staleness/liveness P1 high-priority
Projects
None yet
Development

No branches or pull requests

3 participants