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 logic when querying channels needs improvement/refactor #93

Closed
jasenmoloy opened this issue Oct 4, 2019 · 4 comments

Comments

@jasenmoloy
Copy link

commented Oct 4, 2019

The anonymous class that is handling the callback for querying channels has retry logic when an error occurs.

Specifically ChannelListViewModel line 311:

            public void onError(String errMsg, int errCode) {
                Log.e(TAG, "onError for loading the channels " + errMsg);
                if (attempt > 100) {
                    Log.e(TAG, "tried more than 100 times, give up now");
                    return;
                }
                if (!client().isConnected()) {
                    return;
                }
                int sleep = Math.min(500 * (attempt * attempt + 1), 30000);
                Log.d(TAG, "retrying in " + sleep);
                retryLooper.postDelayed(() -> {
                    queryChannelsInner(attempt + 1);
                }, sleep);
            }

The logic in this onError() method does not log an error occurs until 100 attempts are made. That's a significant amount of time before it's logged. What's the reason for such a high number of attempts?

This is related to #92 where it would be nice to inform the SDK client that an error occurs so we can present UI to the user that something went wrong.

@tbarbugli

This comment has been minimized.

Copy link
Member

commented Oct 4, 2019

Currently the lib does something reasonable by default but it is true that exposing retry logic makes a lot of sense.

The change is fairly small, I am just waiting for a PR to get merged before we get this one done.

@jasenmoloy @tschellenbach what do you think is the friendlies thing to do for the library? I was thinking maybe we allow you to pass something like an interceptor function with this signature: ResponseError error, int attemptCount and something like a stop function. With that you can decide what to do about the error and if you want to stop retrying just call Stop()

@tbarbugli tbarbugli self-assigned this Oct 4, 2019
@jasenmoloy

This comment has been minimized.

Copy link
Author

commented Oct 7, 2019

My concern with having retry logic along with an exposed callback is the inflexibility. Depending on the error, we probably won't retry at all. Or we could present the user with a retry button if say they are in airplane mode.

Having retry logic in the library with an exposed callback also brings up more edge cases. For example, I don't see the retryLooper's runnables getting canceled if there is a lifecycle change. We'd want to stop all potential network calls if the user leaves the app.

@tschellenbach

This comment has been minimized.

Copy link
Member

commented Oct 7, 2019

Totally agree, do you have an example of how you've seen other libraries make this customizable? We can come up with something, but if you have an idea of how you'd like it to work let us know

@tschellenbach

This comment has been minimized.

Copy link
Member

commented Oct 7, 2019

Took a quick pass at it, what do you think?
#109

Also made it easier to subclass the viewModel in case you want to change anything else in the business logic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.