Skip to content

fix(ffi): add methods to make QR login handlers exit cooperatively#6677

Open
Johennes wants to merge 11 commits into
matrix-org:mainfrom
Johennes:johannes/cooperation
Open

fix(ffi): add methods to make QR login handlers exit cooperatively#6677
Johennes wants to merge 11 commits into
matrix-org:mainfrom
Johennes:johannes/cooperation

Conversation

@Johennes

@Johennes Johennes commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

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.

  • I've documented the public API changes in the appropriate changelog files (see Writing changelog entries).
  • This PR was made with the help of AI.

@Johennes Johennes force-pushed the johannes/cooperation branch from 06d5662 to 6d59114 Compare June 19, 2026 15:58
@codecov

codecov Bot commented Jun 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.91304% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.94%. Comparing base (374ed78) to head (0107945).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...atrix-sdk/src/authentication/oauth/qrcode/login.rs 91.42% 13 Missing and 2 partials ⚠️
...atrix-sdk/src/authentication/oauth/qrcode/grant.rs 95.01% 5 Missing and 8 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6677      +/-   ##
==========================================
+ Coverage   89.91%   89.94%   +0.03%     
==========================================
  Files         398      398              
  Lines      111154   111482     +328     
  Branches   111154   111482     +328     
==========================================
+ Hits        99948   100278     +330     
+ Misses       7415     7410       -5     
- Partials     3791     3794       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@codspeed-hq

codspeed-hq Bot commented Jun 19, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 50 untouched benchmarks


Comparing Johennes:johannes/cooperation (0107945) with main (374ed78)

Open in CodSpeed

Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
@Johennes Johennes force-pushed the johannes/cooperation branch from 6d59114 to a2f3f61 Compare June 19, 2026 17:07
@Johennes Johennes marked this pull request as ready for review June 19, 2026 17:15
@Johennes Johennes requested a review from a team as a code owner June 19, 2026 17:15
@Johennes Johennes requested review from Hywan and removed request for a team June 19, 2026 17:15

@Hywan Hywan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread bindings/matrix-sdk-ffi/src/qr_code.rs Outdated
Comment thread bindings/matrix-sdk-ffi/src/qr_code.rs Outdated
Comment thread bindings/matrix-sdk-ffi/src/qr_code.rs Outdated
Comment thread bindings/matrix-sdk-ffi/src/qr_code.rs Outdated
Johennes added 2 commits June 23, 2026 20:34
…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>
@Johennes

Copy link
Copy Markdown
Contributor Author

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.

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.

@Johennes Johennes requested a review from Hywan June 23, 2026 18:50
@Hywan

Hywan commented Jun 25, 2026

Copy link
Copy Markdown
Member

The problem only exists because of the progress tasks and those only exist in the FFI crate.

Yes, but there is no interaction between the cancellation and the progress listener. I think it's a safe feature to move into matrix-sdk.

Or, alternatively (I don't want and I don't like this solution but I'm providing an alternative here), add tests in matrix-sdk-ffi for that. But please, consider moving this code (plus adding a test) in matrix-sdk first.

…vely

Revert back to biased and document why its needed

Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>

@Hywan Hywan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One feedback unaddressed (about moving it to matrix-sdk).

@Johennes

Copy link
Copy Markdown
Contributor Author

One feedback unaddressed (about moving it to matrix-sdk).

Yes, apologies. I'm still looking into that.

@Hywan

Hywan commented Jun 29, 2026

Copy link
Copy Markdown
Member

No problem!

…vely

Move logic into SDK

Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
@Johennes

Johennes commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Sorry for the long wait. I have moved the logic into the SDK in b731790. This is still missing tests in the granting case and updated changelog entries. But is this what you had in mind?

@Johennes Johennes requested a review from Hywan July 2, 2026 19:06
@Hywan

Hywan commented Jul 3, 2026

Copy link
Copy Markdown
Member

Looks like it's the right direction.

Johennes added 4 commits July 6, 2026 19:18
…vely

Add tests for the grant case

Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
…vely

Update changelogs

Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
…vely

Add missing use declarations

Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
@Johennes

Johennes commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Somehow GitHub seems to have lost your comment about Notify vs. CancellationToken. I ran into ownership issues with Notify because it doesn't implement Clone. It's already used elsewhere in the project so I hoped it was ok to reuse here.

Johennes added 2 commits July 6, 2026 19:50
…vely

Add more missing use declarations

Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
…vely

Add missing semicolon

Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
@Johennes

Johennes commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Otherwise, this should finally be ready for review now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants