Refactor FxA state machine to simplify transitions#7359
Conversation
skhamis
force-pushed
the
fxa-state-machine-refactor
branch
from
6b5e283 to
bd490c0
Compare
bendk
left a comment
bendk
left a comment
There was a problem hiding this comment.
I like this approach a lot, it feels way more direct than having the internal states implicitly trigger a FirefoxAccount method call. I just left some comments/questions about the details.
| impl StateMachineErr { | ||
| /// Programming errors (logic / invalid-transition) become `Fatal` and | ||
| /// ignore `target_if_handled`; everything else becomes `Handled`. | ||
| pub fn from_cause(cause: Error, target_if_handled: FxaState) -> Self { |
There was a problem hiding this comment.
Nit: StateMachineErr:from_cause() and Result<T, StateMachineError>::err_state() do pretty much the same thing but have very different names. I can't think of a good single name, but maybe we could get a pair of names that seem related like StateMachineErr::new() and Result<T, StateMachineError>::map_err_to_state_machine_err() . I don't really love those names either though, so whatever you want to go with is good with me.
There was a problem hiding this comment.
ah good callouts, I'll go to ::new() as that feels the most "rust-y". I feel like for chaining on the result maybe going to_state_machine_err has the strongest readability imo.
skhamis
force-pushed
the
fxa-state-machine-refactor
branch
from
bd490c0 to
2ceb842
Compare
|
@mhammond since you also recently used the FxA state machine/authorization I'd love to also get your thoughts to ensure we're moving in the right direction here. |
mhammond
left a comment
mhammond
left a comment
There was a problem hiding this comment.
this is a nice improvement, thanks!
| FxaError::Authentication | ||
| if self.policy.auth_retry_with_cache_clear && !auth_retried => | ||
| { | ||
| self.inner.clear_access_token_cache(); |
There was a problem hiding this comment.
I'm actually skeptical this is needed here (I don't think access tokens are used by the state machine?) - but I see it already existed, so I wont get too distracted by it :)
| .to_state_machine_err(|| S::AuthIssues)?; | ||
| Ok(if active { S::Connected } else { S::AuthIssues }) | ||
| } | ||
| Err(cause) => Err(StateMachineErr::new(cause, S::Disconnected)), |
There was a problem hiding this comment.
this seems a little wrong (and may also be wrong currently) - if ensure_capabilities() fails due to a non-auth error (eg, a network error), the state becomes disconnected? Shouldn't this use something like with_auth_recovery where there's a bit of retry logic (even though, as I mentioned, clearing the access token cache and retrying isn't going to help in practice because ensure_capabilities() doesn't use an access token).
And for the is_auth_error case, it also seems wrong that failure to ensure capabilities due to an auth error ends up in the Connected state when we haven't set the device capabilities?
There was a problem hiding this comment.
I definitely agree with this, I think this makes sense to prioritize in the follow-up where we're removing some of the public APIs and then we can start changing what we're actually doing in the state machine. Right now I'm leaning towards just keeping the patch for simplifying and consolidating just incase we need to think through what actually happens here (and also maybe removing clearing the access token too)
skhamis
force-pushed
the
fxa-state-machine-refactor
branch
from
2ceb842 to
c730c06
Compare
3 participants
Now that FxiOS has the new state machine, we're able to now re-look at the state machine system as a whole and we realized we can simplify it now that we're not merging with existing systems on the consumers. My main goals were:
@bendk had to manage two state machines in android which was the reason for the initial system, but now that they've migrated fully over, the idea came that we can remove the internal state completely and move to a flat-state-machine style. It's in draft now as I think this is a strong denature and want to get thoughts on this before moving on.
Pull Request checklist
[ci full]to the PR title.