Skip to content

Commit

Permalink
Address @touilleMan's comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@vxgmichel
vxgmichel committed Sep 3, 2024
1 parent 9ebdad0 commit d5448b7
Show file tree
Hide file tree
Showing 3 changed files with 112 additions and 104 deletions.
80 changes: 52 additions & 28 deletions libparsec/crates/client/src/invite/claimer.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,14 +100,37 @@ async fn cancel_greeting_attempt(
timestamp,
}),
// Unexpected errors
Rep::GreetingAttemptNotFound => Err(anyhow::anyhow!("Greeting attempt not found").into()),
Rep::GreetingAttemptNotJoined => Err(anyhow::anyhow!("Greeting attempt not joined").into()),
rep @ Rep::UnknownStatus { .. } => {
Rep::UnknownStatus { .. }
| Rep::GreetingAttemptNotFound
| Rep::GreetingAttemptNotJoined => {
Err(anyhow::anyhow!("Unexpected server response: {:?}", rep).into())
}
}
}

// Cancel the greeting attempt and log a warning if an error occurs.
// This is used when the peer has already detected an issue and tries to communicate
// the reason of this issue to the other peer. If this request fails, there is
// no reason to try again or deal with the error in a specific way. Instead, the caller
// simply propagates the error that originally caused the greeting attempt to be cancelled.
// The greeting attempt will then be automatically cancelled when a new one is started.
// Only the reason of the cancellation is lost, which is not a big deal as it was only
// meant to be informative and to improve the user experience.
async fn cancel_greeting_attempt_and_warn_on_error(
cmds: &InvitedCmds,
greeting_attempt: GreetingAttemptID,
reason: CancelledGreetingAttemptReason,
) {
if let Err(err) = cancel_greeting_attempt(cmds, greeting_attempt, reason).await {
log::warn!(
"Claimer failed to cancel greeting attempt {:?} with reason {:?}: {:?}",
greeting_attempt,
reason,
err
);
}
}

// Greeter step helper

async fn run_claimer_step_until_ready(
Expand Down Expand Up @@ -151,19 +174,11 @@ async fn run_claimer_step_until_ready(
timestamp,
}),
// Unexpected errors
invite_claimer_step::Rep::GreetingAttemptNotFound => {
Err(anyhow::anyhow!("Greeting attempt not found").into())
}
invite_claimer_step::Rep::GreetingAttemptNotJoined => {
Err(anyhow::anyhow!("Greeting attempt not joined").into())
}
invite_claimer_step::Rep::StepMismatch => {
Err(anyhow::anyhow!("Greeting attempt failed due to step mismatch").into())
}
invite_claimer_step::Rep::StepTooAdvanced => {
Err(anyhow::anyhow!("Greeting attempt failed due to step too advanced").into())
}
rep @ invite_claimer_step::Rep::UnknownStatus { .. } => {
invite_claimer_step::Rep::UnknownStatus { .. }
| invite_claimer_step::Rep::GreetingAttemptNotFound
| invite_claimer_step::Rep::GreetingAttemptNotJoined
| invite_claimer_step::Rep::StepMismatch
| invite_claimer_step::Rep::StepTooAdvanced => {
Err(anyhow::anyhow!("Unexpected server response: {:?}", rep).into())
}
};
Expand Down Expand Up @@ -246,6 +261,15 @@ impl ClaimCancellerCtx {
)
.await
}

pub async fn cancel_and_warn_on_error(self) {
cancel_greeting_attempt_and_warn_on_error(
&self.cmds,
self.greeting_attempt,
CancelledGreetingAttemptReason::ManuallyCancelled,
)
.await;
}
}

#[derive(Debug)]
Expand Down Expand Up @@ -783,12 +807,12 @@ impl UserClaimInProgress3Ctx {
}
_ => CancelledGreetingAttemptReason::InconsistentPayload,
};
if cancel_greeting_attempt(&self.0.cmds, self.0.greeting_attempt, reason)
.await
.is_err()
{
// TODO: Warn about the error before discarding it
};
cancel_greeting_attempt_and_warn_on_error(
&self.0.cmds,
self.0.greeting_attempt,
reason,
)
.await;
return Err(ClaimInProgressError::CorruptedConfirmation(err));
}
};
Expand Down Expand Up @@ -874,12 +898,12 @@ impl DeviceClaimInProgress3Ctx {
}
_ => CancelledGreetingAttemptReason::InconsistentPayload,
};
if cancel_greeting_attempt(&self.0.cmds, self.0.greeting_attempt, reason)
.await
.is_err()
{
// TODO: Warn about the error before discarding it
};
cancel_greeting_attempt_and_warn_on_error(
&self.0.cmds,
self.0.greeting_attempt,
reason,
)
.await;
return Err(ClaimInProgressError::CorruptedConfirmation(err));
}
};
Expand Down
80 changes: 46 additions & 34 deletions libparsec/crates/client/src/invite/greeter.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -294,14 +294,37 @@ async fn cancel_greeting_attempt(
timestamp,
}),
// Unexpected errors
Rep::GreetingAttemptNotFound => Err(anyhow::anyhow!("Greeting attempt not found").into()),
Rep::GreetingAttemptNotJoined => Err(anyhow::anyhow!("Greeting attempt not joined").into()),
rep @ Rep::UnknownStatus { .. } => {
Rep::UnknownStatus { .. }
| Rep::GreetingAttemptNotFound
| Rep::GreetingAttemptNotJoined => {
Err(anyhow::anyhow!("Unexpected server response: {:?}", rep).into())
}
}
}

// Cancel the greeting attempt and log a warning if an error occurs.
// This is used when the peer has already detected an issue and tries to communicate
// the reason of this issue to the other peer. If this request fails, there is
// no reason to try again or deal with the error in a specific way. Instead, the caller
// simply propagates the error that originally caused the greeting attempt to be cancelled.
// The greeting attempt will then be automatically cancelled when a new one is started.
// Only the reason of the cancellation is lost, which is not a big deal as it was only
// meant to be informative and to improve the user experience.
async fn cancel_greeting_attempt_and_warn_on_error(
cmds: &AuthenticatedCmds,
greeting_attempt: GreetingAttemptID,
reason: CancelledGreetingAttemptReason,
) {
if let Err(err) = cancel_greeting_attempt(cmds, greeting_attempt, reason).await {
log::warn!(
"Greeter failed to cancel greeting attempt {:?} with reason {:?}: {:?}",
greeting_attempt,
reason,
err
);
}
}

// Greeter step helper

async fn run_greeter_step_until_ready(
Expand Down Expand Up @@ -348,19 +371,11 @@ async fn run_greeter_step_until_ready(
timestamp,
}),
// Unexpected errors
invite_greeter_step::Rep::GreetingAttemptNotFound => {
Err(anyhow::anyhow!("Greeting attempt not found").into())
}
invite_greeter_step::Rep::GreetingAttemptNotJoined => {
Err(anyhow::anyhow!("Greeting attempt not joined").into())
}
invite_greeter_step::Rep::StepMismatch => {
Err(anyhow::anyhow!("Greeting attempt failed due to step mismatch").into())
}
invite_greeter_step::Rep::StepTooAdvanced => {
Err(anyhow::anyhow!("Greeting attempt failed due to step too advanced").into())
}
rep @ invite_greeter_step::Rep::UnknownStatus { .. } => {
invite_greeter_step::Rep::UnknownStatus { .. }
| invite_greeter_step::Rep::GreetingAttemptNotFound
| invite_greeter_step::Rep::GreetingAttemptNotJoined
| invite_greeter_step::Rep::StepMismatch
| invite_greeter_step::Rep::StepTooAdvanced => {
Err(anyhow::anyhow!("Unexpected server response: {:?}", rep).into())
}
};
Expand All @@ -384,6 +399,15 @@ impl GreetCancellerCtx {
)
.await
}

pub async fn cancel_and_warn_on_error(self) {
cancel_greeting_attempt_and_warn_on_error(
&self.cmds,
self.greeting_attempt,
CancelledGreetingAttemptReason::ManuallyCancelled,
)
.await;
}
}

// GreetInitialCtx
Expand Down Expand Up @@ -520,16 +544,12 @@ impl BaseGreetInitialCtx {
};

if HashDigest::from_data(&claimer_nonce) != claimer_hashed_nonce {
if cancel_greeting_attempt(
cancel_greeting_attempt_and_warn_on_error(
&self.cmds,
greeting_attempt,
CancelledGreetingAttemptReason::InvalidNonceHash,
)
.await
.is_err()
{
// TODO: Warn about the error before discarding it
};
.await;
return Err(GreetInProgressError::NonceMismatch);
}

Expand Down Expand Up @@ -870,12 +890,8 @@ impl UserGreetInProgress3Ctx {
}
_ => CancelledGreetingAttemptReason::InconsistentPayload,
};
if cancel_greeting_attempt(&ctx.cmds, ctx.greeting_attempt, reason)
.await
.is_err()
{
// TODO: Warn about the error before discarding it
};
cancel_greeting_attempt_and_warn_on_error(&ctx.cmds, ctx.greeting_attempt, reason)
.await;
return Err(GreetInProgressError::CorruptedInviteUserData(err));
}
};
Expand Down Expand Up @@ -921,12 +937,8 @@ impl DeviceGreetInProgress3Ctx {
}
_ => CancelledGreetingAttemptReason::InconsistentPayload,
};
if cancel_greeting_attempt(&ctx.cmds, ctx.greeting_attempt, reason)
.await
.is_err()
{
// TODO: Warn about the error before discarding it
};
cancel_greeting_attempt_and_warn_on_error(&ctx.cmds, ctx.greeting_attempt, reason)
.await;
return Err(GreetInProgressError::CorruptedInviteUserData(err));
}
};
Expand Down
Loading

0 comments on commit d5448b7

Please sign in to comment.