fix(ffi): add methods to make QR login handlers exit cooperatively#6677
fix(ffi): add methods to make QR login handlers exit cooperatively#6677Johennes wants to merge 3 commits into
Conversation
Johennes
force-pushed
the
johannes/cooperation
branch
from
06d5662 to
6d59114
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6677 +/- ##
=======================================
Coverage 89.86% 89.87%
=======================================
Files 396 396
Lines 110260 110260
Branches 110260 110260
=======================================
+ Hits 99088 99098 +10
+ Misses 7395 7391 -4
+ Partials 3777 3771 -6 ☔ View full report in Codecov by Harness. |
Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Johennes
force-pushed
the
johannes/cooperation
branch
from
6d59114 to
a2f3f61
Compare
Hywan
left a comment
Hywan
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I think it makes sense but I've added a couple comments.
Would it be possible to move that inside the matrix_sdk crate? First off, it would allow proper testing, and second, it can be useful for Rust users too. Keep in mind the FFI crate is only about bindings, no logic at all.
| pub struct LoginWithQrCodeHandler { | ||
| oauth: OAuth, | ||
| oauth_configuration: OAuthConfiguration, | ||
| /// Request a the handler to abort cooperatively. This will make the handler |
|
|
||
| Ok(()) | ||
| tokio::select! { | ||
| biased; |
There was a problem hiding this comment.
If it is biased, why do you want to give priority to login instead of self.cancel.notified()? Cancelling seems more important here, don't you think?
Please add a comment on biased to justify and explain it.
There was a problem hiding this comment.
Good point. On second thought, I think we don't actually need biasing here. The update from the QR flows depend on user interaction. They should come in sparsely which means biasing shouldn't really make any difference. Have removed it in e9a461d.
| } | ||
|
|
||
| Ok(()) | ||
| /// Request a the handler to abort cooperatively. This will make the handler |
| _ = self.cancel.notified() => { | ||
| // Stop forwarding progress to the foreign callback before tearing | ||
| // down the handler. | ||
| drop(progress_task); |
There was a problem hiding this comment.
Don't you want to emit a progress (on progress_listener) to stay it's been cancelled?
There was a problem hiding this comment.
I think not. The existing pattern with the handler is that any errors are returned via the method's result rather than the update listener. That's why the state enums don't contain any variants for errors or cancellation. So I think just returning an error fits in with the pattern (though you might of course question the pattern itself).
…vely Fix doc comments Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
…vely Remove biasing Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
That would be great but I'm not entirely sure how. The problem only exists because of the progress tasks and those only exist in the FFI crate. |
2 participants
With my Filament hat on: This add new methods on the QR login handlers to make them exit cooperatively by first dropping the updates task and then returning. We've found this necessary when using the React Native FFI bindings in release builds to prevent crashes when the futures are cancelled.