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.

Sign up for GitHub

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

fix: Add retry logic in "_waitForPkamAuthSuccess" to retry when secondary is temporarily not reachable #684

Merged
merged 4 commits into from
Oct 8, 2024
Merged

fix: Add retry logic in "_waitForPkamAuthSuccess" to retry when secondary is temporarily not reachable #684

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
502d708
fix: Add retry logic in "_waitForPkamAuthSuccess" to retry when secon…
sitaram-kalluri Oct 7, 2024
f5e2b3f
fix: Add dart docs and modify the code
sitaram-kalluri Oct 8, 2024
a612f70
fix: Modify error message to fix failing functional test
sitaram-kalluri Oct 8, 2024
cf24f66
fix: Remove _attemptPkamAuth method and invoke pkamAuthenticate in _w…
sitaram-kalluri Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 18 additions & 7 deletions packages/at_onboarding_cli/lib/src/onboard/at_onboarding_service_impl.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ class AtOnboardingServiceImpl implements AtOnboardingService {
AtSignLogger logger = AtSignLogger('OnboardingCli');
AtOnboardingPreference atOnboardingPreference;
AtLookUp? _atLookUp;
final _maxActivationRetries = 5;

/// The object which controls what types of AtClients, NotificationServices
/// and SyncServices get created when we call [AtClientManager.setCurrentAtSign].
Expand Down Expand Up @@ -321,15 +322,28 @@ class AtOnboardingServiceImpl implements AtOnboardingService {
Duration retryInterval, {
bool logProgress = true,
}) async {
int retryAttempt = 1;
while (true) {
logger.info('Attempting pkam auth');
if (logProgress) {
stderr.write('Checking ... ');
}
bool pkamAuthSucceeded = await _attemptPkamAuth(
atLookUp,
enrollmentIdFromServer,
);
bool pkamAuthSucceeded = false;
try {
pkamAuthSucceeded =
await _attemptPkamAuth(atLookUp, enrollmentIdFromServer);
} catch (e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does _attemptPkamAuth throw an exception if PKAM auth fails in a "normal" way (i.e. the enrollment has not yet been approved)?

Some code comments explaining what exactly is going on here would be useful

Copy link
Member Author

@sitaram-kalluri sitaram-kalluri Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the _attemptPkamAuth method, if enrollment is denied, it returns an AtEnrollmentException. This exception is caught in the catch block of _waitForPkamAuthSuccess (which calls _attemptPkamAuth),
triggering the retry logic that we want to avoid. To prevent this, we need to handle AtEnrollmentException separately here, which introduces code duplication.

To handle all the exceptions in one place, moved the existing exception handing logic in _attemptPkamAuth here. With the changes, _attemptPkamAuth do not handle exception. It rethrows and all the exceptions are handled in the _waitForPkamAuthSuccess.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK - I'd suggest we now remove _attemptPkamAuth and move its code inline, as _attemptPkamAuth isn't used anywhere else

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the "_attemptPkamAuth" and moved its code inline.

String message =
'Exception occurred when authenticating the atSign: $_atSign caused by ${e.toString()}';
if (retryAttempt > _maxActivationRetries) {
message += ' Activation failed after $_maxActivationRetries attempts';
logger.severe(message);
rethrow;
}
logger
.severe('$message. Attempting to retry for $retryAttempt attempt');
retryAttempt++;
}
if (pkamAuthSucceeded) {
if (logProgress) {
stderr.writeln(' approved.');
Expand Down Expand Up @@ -366,9 +380,6 @@ class AtOnboardingServiceImpl implements AtOnboardingService {
} else if (e.message.contains('error:AT0025')) {
throw AtEnrollmentException('enrollment denied');
}
} catch (e) {
logger.shout('Unexpected exception: $e');
rethrow;
} finally {
logger.finer('_attemptPkamAuth: complete');
}
Expand Down