Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid accessing realm instance from suspend functions #5592

Conversation

ariskotsomitopoulos
Copy link
Contributor

Add runBlocking when decrypt events to avoid thread switching when accessing the realm instance (thread local)

@ariskotsomitopoulos ariskotsomitopoulos changed the title Add runBlocking when decrypt events to avoid thread switching when ac… Avoid accessing realm instance from suspend functions Mar 21, 2022
@github-actions
Copy link

github-actions bot commented Mar 21, 2022

Unit Test Results

114 files  ±0  114 suites  ±0   1m 35s ⏱️ +19s
202 tests ±0  202 ✔️ ±0  0 💤 ±0  0 ±0 
678 runs  ±0  678 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 5cfe218. ± Comparison against base commit f5973fa.

♻️ This comment has been updated with latest results.

@@ -101,9 +101,7 @@ internal class TimelineEventDecryptor @Inject constructor(
executor?.execute {
Realm.getInstance(realmConfiguration).use { realm ->
try {
runBlocking {
Copy link
Contributor

Choose a reason for hiding this comment

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

if the aim is to ensure the realm instance is only executed on a single thread it might be safer to keep the runBlocking here next to the instance creation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to avoid making the function processDecryptRequest suspended. However, I can see the benefit you describe.

@ouchadam
Copy link
Contributor

it's a bit tricky to tell from the diff, where does the thread jumping happen?

I was expecting to see a withContext or scope.launch being replaced/removed

@ariskotsomitopoulos
Copy link
Contributor Author

ariskotsomitopoulos commented Mar 21, 2022

From my understanding without suspend function, no thread hopping can be done. Maybe @BillCarsonFr has a better input on that.

Copy link
Member

@BillCarsonFr BillCarsonFr left a comment

Choose a reason for hiding this comment

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

I would just wrap cryptoService.decryptEvent(XXX) with runBlocking instead of whole functions

…nd_functions

# Conflicts:
#	matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/timeline/TimelineEventDecryptor.kt
#	matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/sync/handler/room/RoomSyncHandler.kt
@ariskotsomitopoulos ariskotsomitopoulos merged commit 09e8c10 into develop Apr 27, 2022
@ariskotsomitopoulos ariskotsomitopoulos deleted the feature/aris/prevent_decryption_fom_suspend_functions branch April 27, 2022 14:09
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.

3 participants