fix(ffi): Room::is_encrypted is now async
#4756
Conversation
This patch updates `Room::is_encrypted` to be async, otherwise it could block forever in case of bad network or offline network.
Hywan
requested review from
andybalaam and
stefanceriu
and removed request for
a team and
andybalaam
| @@ -257,8 +257,8 @@ impl Room { | |||
| self.inner.room_id().to_string() | |||
| } | |||
|
|
|||
| pub fn is_encrypted(&self) -> Result<bool, ClientError> { | |||
| Ok(RUNTIME.block_on(self.inner.is_encrypted())?) | |||
| pub async fn is_encrypted(&self) -> Result<bool, ClientError> { | |||
There was a problem hiding this comment.
AFAIK Android will be quite unhappy about that. So please ask them before merging.
There was a problem hiding this comment.
Yeah, we do rely on this value to be synced, it's really unfortunate that this value is not part of RoomInfo
There was a problem hiding this comment.
@ganfra Well, it's blocking the iOS team. It's a bit annoying. Can't it be async on your side (block the call or something)?
There was a problem hiding this comment.
The question is, could we get this value in RoomInfo as an optional? So this value can get refreshed automatically
There was a problem hiding this comment.
RoomInfo::new in matrix-sdk-ffi is async, so it should be possible, but I remember we have removed is_encrypted from it because it could have sent a request, which would create a lag in some cases. That's why we have extracted it in Room::is_encrypted directly.
See this patch from @jmartinesp: ad4d24f
There was a problem hiding this comment.
Also, @jmartinesp did introduce this feature one patch earlier with 004941b.
There was a problem hiding this comment.
Sure, thats why I'm asking if we can this value as optional, so if we don't have the info, just trigger a background request which updates the room info asynchronously
There was a problem hiding this comment.
@ganfra You are asking a field that is Option<bool>:
Some(true)if it's synced and the room is encryptedSome(false)if it's synced and the room is NOT encryptedNoneif it's not synced (and you sync yourself)
Is that what you're asking?
Is the SDK user (so Android in this case) to run the request and update RoomInfo asynchronously? How do you imagine handling that?
There was a problem hiding this comment.
Yes it's what I'm asking, or it could also be an enum if it's clearer.
And yes, I'd expect the client to run the request whenever he needs to (ie. probably when retrieving None). I'd just expect the sdk to ensure only one request is made at a time.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4756 +/- ##
=======================================
Coverage 86.12% 86.12%
=======================================
Files 292 292
Lines 34346 34346
=======================================
Hits 29581 29581
Misses 4765 4765 ☔ View full report in Codecov by Sentry. |
- this came from having seen the opening room modal show up too often which was caused by `isEncrypted` being block_on on the rust side and sometimes making network requests - we aggreed that the `block_on`s should go, which is handled in matrix-org/matrix-rust-sdk#4756 and matrix-org/matrix-rust-sdk#4757
- this came from having seen the opening room modal show up too often which was caused by `isEncrypted` being block_on on the rust side and sometimes making network requests - we aggreed that the `block_on`s should go, which is handled in matrix-org/matrix-rust-sdk#4756 and matrix-org/matrix-rust-sdk#4757
This patch updates
Room::is_encryptedto be async, otherwise it could block forever in case of bad network or offline network.