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

Unable to change avatar due to NetworkOnMainThread #4783

Merged
merged 3 commits into from
Dec 30, 2021

Conversation

ouchadam
Copy link
Contributor

Fixes #4767 Unable to change avatar

We had a report of NetworkOnMainThread whilst changing avatar, I've been unable to reproduce the issue but did notice we have an unnecessary main thread dispatcher being used (looks like it was introduced as part of the suspend migration).

  • Switches the main dispatcher to io, the user has also confirmed this fixed their issue
  • Bundles the file reading + temp file creation into the same io context
changing-avatar

@@ -68,7 +68,7 @@ internal class DefaultProfileService @Inject constructor(private val taskExecuto
}

override suspend fun updateAvatar(userId: String, newAvatarUri: Uri, fileName: String) {
withContext(coroutineDispatchers.main) {
withContext(coroutineDispatchers.io) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the fix

Copy link
Member

Choose a reason for hiding this comment

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

Its something we already had fixed, not sure why its back. But fileUploader should already dispatch on a background thread, its weird

Copy link
Contributor Author

@ouchadam ouchadam Dec 22, 2021

Choose a reason for hiding this comment

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

yeah it's very strange, I checked the okhttp thread pool/interceptors as well, couldn't find any cases where the main thread was being used, unless on some devices the internal okhttp thread uses the main thread/looper for some reason 🤔

Copy link
Member

Choose a reason for hiding this comment

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

This is also something which has been fixed by some fork on their side FWIW.

@github-actions
Copy link

Unit Test Results

  66 files  ±0    66 suites  ±0   51s ⏱️ -3s
135 tests ±0  135 ✔️ ±0  0 💤 ±0  0 ±0 
418 runs  ±0  418 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit a764e02. ± Comparison against base commit 55c0f1f.

Copy link
Member

@ganfra ganfra left a comment

Choose a reason for hiding this comment

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

Thanks!

@bmarty bmarty merged commit 5407c84 into develop Dec 30, 2021
@bmarty bmarty deleted the feature/adm/unable-to-change-avatar branch December 30, 2021 11:30
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.

It is not possiable to changing user avatar
3 participants