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

fix: image loading when using markdown img and bigImageUrl #353

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

jmattheis
Copy link
Member

Fixes #333

When receiving a message with the same image in the markdown body and in the extras client::notification.bigImageUrl, then there is a clash on the file system.

One request succeeds and the other fails with the following error. This commit ensures that there is only one coil image loader instance, so that there shouldn't be file system race conditions.

WebSocket(1): received message {"id":845,"appid":21,...}
Failed - http://192.168.178.2:8000/1.jpg?v=1718369188 - java.lang.IllegalStateException: closed
java.lang.IllegalStateException: closed
    at okio.RealBufferedSource.rangeEquals(RealBufferedSource.kt:466)
    at okio.RealBufferedSource.rangeEquals(RealBufferedSource.kt:130)
    at coil.decode.SvgDecodeUtils.isSvg(DecodeUtils.kt:19)
    at coil.decode.SvgDecoder$Factory.isApplicable(SvgDecoder.kt:104)
    at coil.decode.SvgDecoder$Factory.create(SvgDecoder.kt:99)
    at coil.ComponentRegistry.newDecoder(ComponentRegistry.kt:100)
    at coil.intercept.EngineInterceptor.decode(EngineInterceptor.kt:197)
    at coil.intercept.EngineInterceptor.access$decode(EngineInterceptor.kt:42)
    at coil.intercept.EngineInterceptor$execute$executeResult$1.invokeSuspend(EngineInterceptor.kt:131)
    at coil.intercept.EngineInterceptor$execute$executeResult$1.invoke(EngineInterceptor.kt)
    at coil.intercept.EngineInterceptor$execute$executeResult$1.invoke(EngineInterceptor.kt)
    at kotlinx.coroutines.intrinsics.UndispatchedKt.startUndispatchedOrReturn(Undispatched.kt:78)
    at kotlinx.coroutines.BuildersKt__Builders_commonKt.withContext(Builders.common.kt:167)
    at kotlinx.coroutines.BuildersKt.withContext(Unknown Source)
    at coil.intercept.EngineInterceptor.execute(EngineInterceptor.kt:130)
    at coil.intercept.EngineInterceptor.access$execute(EngineInterceptor.kt:42)
    at coil.intercept.EngineInterceptor$execute$1.invokeSuspend(EngineInterceptor.kt)
    at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
    at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:108)
    at kotlinx.coroutines.internal.LimitedDispatcher$Worker.run(LimitedDispatcher.kt:115)
    at kotlinx.coroutines.scheduling.TaskImpl.run(Tasks.kt:103)
    at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:584)
    at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:793)
    at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:697)
    at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:684)
Successful (NETWORK) - http://192.168.178.2:8000/1.jpg?v=1718369188

The problem is reproducible: host a big image (20mb or more nearly trigger the bug 100%) on a server. When this message

{
  "title": "Title",
  "message": "![](http://192.168.178.2:8000/1.jpg?v=VERSION)",
  "priority": 5,
  "extras": {
    "client::display": {
      "contentType": "text/markdown"
    },
    "client::notification": {
      "bigImageUrl": "http://192.168.178.2:8000/1.jpg?v=VERSION"
    }
  }
}

is viewed on android, then either the bigImageUrl or the markdown image doesn't load correctly. Caching suppresses the issue, so replace VERSION with something unique, so the bug is triggered on every POST.

cat msg.json| sed -E "s/VERSION/`date +%s%N`/g" |http POST ':8080/message?token=TOKEN'

After this commit, I couldn't reproduce the problem anymore.

When receiving a message with the same image in the markdown body and in
the extras client::notification.bigImageUrl, then there is a clash on
the file system.

One request succeeds and the other fails with the following error. This
commit ensures that there is only one coil image loader instance, so
that there shouldn't be file system race conditions.

    WebSocket(1): received message {"id":845,"appid":21,...}
    Failed - http://192.168.178.2:8000/1.jpg?v=1718369188 - java.lang.IllegalStateException: closed
    java.lang.IllegalStateException: closed
        at okio.RealBufferedSource.rangeEquals(RealBufferedSource.kt:466)
        at okio.RealBufferedSource.rangeEquals(RealBufferedSource.kt:130)
        at coil.decode.SvgDecodeUtils.isSvg(DecodeUtils.kt:19)
        at coil.decode.SvgDecoder$Factory.isApplicable(SvgDecoder.kt:104)
        at coil.decode.SvgDecoder$Factory.create(SvgDecoder.kt:99)
        at coil.ComponentRegistry.newDecoder(ComponentRegistry.kt:100)
        at coil.intercept.EngineInterceptor.decode(EngineInterceptor.kt:197)
        at coil.intercept.EngineInterceptor.access$decode(EngineInterceptor.kt:42)
        at coil.intercept.EngineInterceptor$execute$executeResult$1.invokeSuspend(EngineInterceptor.kt:131)
        at coil.intercept.EngineInterceptor$execute$executeResult$1.invoke(EngineInterceptor.kt)
        at coil.intercept.EngineInterceptor$execute$executeResult$1.invoke(EngineInterceptor.kt)
        at kotlinx.coroutines.intrinsics.UndispatchedKt.startUndispatchedOrReturn(Undispatched.kt:78)
        at kotlinx.coroutines.BuildersKt__Builders_commonKt.withContext(Builders.common.kt:167)
        at kotlinx.coroutines.BuildersKt.withContext(Unknown Source)
        at coil.intercept.EngineInterceptor.execute(EngineInterceptor.kt:130)
        at coil.intercept.EngineInterceptor.access$execute(EngineInterceptor.kt:42)
        at coil.intercept.EngineInterceptor$execute$1.invokeSuspend(EngineInterceptor.kt)
        at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
        at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:108)
        at kotlinx.coroutines.internal.LimitedDispatcher$Worker.run(LimitedDispatcher.kt:115)
        at kotlinx.coroutines.scheduling.TaskImpl.run(Tasks.kt:103)
        at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:584)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:793)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:697)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:684)
    Successful (NETWORK) - http://192.168.178.2:8000/1.jpg?v=1718369188
@@ -16,56 +16,72 @@ import java.io.IOException
import okhttp3.OkHttpClient
import org.tinylog.kotlin.Logger

internal class CoilHandler(private val context: Context, private val settings: Settings) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@cyb3rko Would you mind reviewing this PR, it's related to #337.

Copy link
Contributor

@cyb3rko cyb3rko Jun 17, 2024

Choose a reason for hiding this comment

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

LGTM.
But the edge cases were not tested because I do not have that kind of setup for testing it atm, I trust you there :)

I've added a smaller comment

Copy link
Member Author

Choose a reason for hiding this comment

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

@cyb3rko Thanks for the review, what smaller comment do you mean?

@jmattheis jmattheis mentioned this pull request Jun 15, 2024
@jmattheis jmattheis merged commit 28698bf into master Jun 18, 2024
3 checks passed
@jmattheis jmattheis deleted the concurrent-image-access branch June 18, 2024 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Loads old, incorrect, picture from it's cache instead of new picture
2 participants