PerUserTopicSubscriptionManager: retry only once on auth errors
On receiving an auth error (HTTP 401), PerUserTopicSubscriptionManager
invalidates the access token and retries, based on the assumption that
the access token was likely expired. That makes sense once, but if the
second request fails with an auth error again, there's really no point
in trying again. In some cases, it might even lead to an infinite loop
of invalidating and re-requesting access tokens.
This CL changes the behavior so that a request is retried exactly once
on an auth error. If it fails again, it is abandoned (which already
happens for all other 4xx errors).
Bug: 1020117
Change-Id: I0c83d3eaa5bafcc8d9f81315921f8375661371af
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2000625
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#732399}
diff --git a/components/invalidation/impl/per_user_topic_subscription_manager.cc b/components/invalidation/impl/per_user_topic_subscription_manager.cc
index e301856..30bebe1 100644
--- a/components/invalidation/impl/per_user_topic_subscription_manager.cc
+++ b/components/invalidation/impl/per_user_topic_subscription_manager.cc
@@ -184,6 +184,8 @@
std::unique_ptr<PerUserTopicSubscriptionRequest> request;
+ bool has_retried_on_auth_error = false;
+
DISALLOW_COPY_AND_ASSIGN(SubscriptionEntry);
};
@@ -423,39 +425,50 @@
PerUserTopicSubscriptionRequest::RequestType type) {
if (code.IsSuccess()) {
ActOnSuccessfulSubscription(topic, private_topic_name, type);
- } else {
- auto it = pending_subscriptions_.find(topic);
- if (code.IsAuthFailure()) {
- // Invalidate previous token, otherwise the identity provider will return
- // the same token again.
- // TODO(crbug.com/1020117): Don't invalidate multiple access tokens in a
- // row - just retry once, then give up. (Or at least use backoff!)
- if (!access_token_.empty()) {
- identity_provider_->InvalidateAccessToken({kFCMOAuthScope},
- access_token_);
- access_token_.clear();
- }
- // Re-request access token and try subscription requests again.
- RequestAccessToken();
- } else {
- // If one of the subscription requests failed, emit SUBSCRIPTION_FAILURE.
- if (type == PerUserTopicSubscriptionRequest::SUBSCRIBE &&
- base::FeatureList::IsEnabled(
- invalidation::switches::kFCMInvalidationsConservativeEnabling)) {
- NotifySubscriptionChannelStateChange(
- SubscriptionChannelState::SUBSCRIPTION_FAILURE);
- }
- if (!code.ShouldRetry()) {
- pending_subscriptions_.erase(it);
- return;
- }
- // TODO(crbug.com/1020117): This should probably go through
- // RequestAccessToken() to make sure we have a fresh one. (The identity
- // code will only actually request a new one from the network if the
- // existing one has expired.)
- ScheduleRequestForRepetition(topic);
- }
+ return;
}
+
+ auto it = pending_subscriptions_.find(topic);
+ // If this is the first auth error we've encountered, then most likely the
+ // access token has just expired. Get a new one and retry immediately.
+ if (code.IsAuthFailure() && !it->second->has_retried_on_auth_error) {
+ it->second->has_retried_on_auth_error = true;
+ // Invalidate previous token, otherwise the identity provider will return
+ // the same token again.
+ if (!access_token_.empty()) {
+ // TODO(crbug.com/1020117): |access_token_| might already be different
+ // from the one we used for this request.
+ identity_provider_->InvalidateAccessToken({kFCMOAuthScope},
+ access_token_);
+ access_token_.clear();
+ }
+ // Re-request access token and try subscription requests again.
+ RequestAccessToken();
+ return;
+ }
+
+ // If one of the subscription requests failed (and we need to either observe
+ // backoff before retrying, or won't retry at all), emit SUBSCRIPTION_FAILURE.
+ if (type == PerUserTopicSubscriptionRequest::SUBSCRIBE &&
+ base::FeatureList::IsEnabled(
+ invalidation::switches::kFCMInvalidationsConservativeEnabling)) {
+ NotifySubscriptionChannelStateChange(
+ SubscriptionChannelState::SUBSCRIPTION_FAILURE);
+ }
+ if (!code.ShouldRetry()) {
+ // Note: This is a pretty bad (and "silent") failure case. The subscription
+ // will generally not be retried until the next Chrome restart (or user
+ // sign-out + re-sign-in).
+ DVLOG(1) << "Got a persistent error while trying to subscribe to topic "
+ << topic << ", giving up.";
+ pending_subscriptions_.erase(it);
+ return;
+ }
+ // TODO(crbug.com/1020117): This should probably go through
+ // RequestAccessToken() to make sure a fresh token is available for the
+ // request. (The identity code will only actually request a new one from the
+ // network if the existing one has expired.)
+ ScheduleRequestForRepetition(topic);
}
TopicSet PerUserTopicSubscriptionManager::GetSubscribedTopicsForTest() const {
diff --git a/components/invalidation/impl/per_user_topic_subscription_manager_unittest.cc b/components/invalidation/impl/per_user_topic_subscription_manager_unittest.cc
index 977782a..66c444a 100644
--- a/components/invalidation/impl/per_user_topic_subscription_manager_unittest.cc
+++ b/components/invalidation/impl/per_user_topic_subscription_manager_unittest.cc
@@ -350,6 +350,59 @@
per_user_topic_subscription_manager->HaveAllRequestsFinishedForTest());
}
+TEST_F(PerUserTopicSubscriptionManagerTest,
+ ShouldInvalidateAccessTokenOnlyOnce) {
+ // For this test, we need to manually control when access tokens are returned.
+ identity_test_env()->SetAutomaticIssueOfAccessTokens(false);
+
+ auto ids = GetSequenceOfTopics(kInvalidationObjectIdsCount);
+
+ auto per_user_topic_subscription_manager = BuildRegistrationManager();
+ ASSERT_TRUE(per_user_topic_subscription_manager->GetSubscribedTopicsForTest()
+ .empty());
+
+ // The first subscription attempt will fail with an "unauthorized" error.
+ AddCorrectSubscriptionResponce(
+ /*private_topic=*/std::string(), kFakeInstanceIdToken,
+ net::HTTP_UNAUTHORIZED);
+
+ per_user_topic_subscription_manager->UpdateSubscribedTopics(
+ ids, kFakeInstanceIdToken);
+ // This should have resulted in a request for an access token. Return one
+ // (which is considered invalid, e.g. already expired).
+ identity_test_env()->WaitForAccessTokenRequestIfNecessaryAndRespondWithToken(
+ "invalid_access_token", base::Time::Now());
+
+ // Now the subscription requests should be scheduled.
+ ASSERT_FALSE(
+ per_user_topic_subscription_manager->HaveAllRequestsFinishedForTest());
+
+ // Wait for the subscription requests to happen.
+ base::RunLoop().RunUntilIdle();
+
+ // Since the subscriptions failed, the requests should still be pending.
+ ASSERT_FALSE(
+ per_user_topic_subscription_manager->HaveAllRequestsFinishedForTest());
+
+ // A new access token should have been requested. (Note: We'd really want to
+ // check that the previous token got invalidated before a new one was
+ // requested, but the identity test code doesn't expose that.)
+ // Serving a new access token will trigger another subscription attempt, but
+ // it'll fail again with the same error.
+ identity_test_env()->WaitForAccessTokenRequestIfNecessaryAndRespondWithToken(
+ "invalid_access_token_2", base::Time::Max());
+ base::RunLoop().RunUntilIdle();
+
+ // On the second auth failure, we should have given up - no new access token
+ // request should have happened, and all the pending subscriptions should have
+ // been dropped, even though still no topics are subscribed.
+ EXPECT_FALSE(identity_test_env()->IsAccessTokenRequestPending());
+ EXPECT_TRUE(per_user_topic_subscription_manager->GetSubscribedTopicsForTest()
+ .empty());
+ EXPECT_TRUE(
+ per_user_topic_subscription_manager->HaveAllRequestsFinishedForTest());
+}
+
TEST_F(PerUserTopicSubscriptionManagerTest, ShouldRepeatRequestsOnForbidden) {
auto ids = GetSequenceOfTopics(kInvalidationObjectIdsCount);
diff --git a/components/invalidation/impl/status.h b/components/invalidation/impl/status.h
index 91bc5d6b..6b766ae 100644
--- a/components/invalidation/impl/status.h
+++ b/components/invalidation/impl/status.h
@@ -46,7 +46,7 @@
bool IsSuccess() const { return code == StatusCode::SUCCESS; }
bool IsAuthFailure() const { return code == StatusCode::AUTH_FAILURE; }
- bool ShouldRetry() const { return code != StatusCode::FAILED_NON_RETRIABLE; }
+ bool ShouldRetry() const { return code == StatusCode::FAILED; }
StatusCode code;
// The message is not meant to be displayed to the user.
