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

Note that observation on multiple threads in layout/draw is not supported. Make sure your measure/layout/draw for each Owner (AndroidComposeView) is executed on the same thread #203

Closed
xemniz opened this issue Nov 4, 2024 · 18 comments · Fixed by #204
Labels
bug Something isn't working question Further information is requested Session Replay

Comments

@xemniz
Copy link

xemniz commented Nov 4, 2024

Version

3.9.0

Steps to Reproduce

  1. I use compose in fragments a lot
  2. It's really ocassional but im just switching tab bars, scrolling lazylists
  3. App just crashes
  4. previousThreadName from the stacktrace is PostHogReplayThread
  5. when i switch off the replay, can't reproduce this

Expected Result

App works

Actual Result

java.lang.IllegalArgumentException: Detected multithreaded access to : previousThreadId=158), currentThread={id=2, name=main}. Note that observation on multiple threads in layout/draw is not supported. Make sure your measure/layout/draw for each Owner (AndroidComposeView) is executed on the same thread.
at androidx.compose.runtime.PreconditionsKt.throwIllegalArgumentException(Preconditions.kt:26)
at androidx.compose.runtime.snapshots.SnapshotStateObserver.observeReads(SnapshotStateObserver.kt:708)
at androidx.compose.ui.node.OwnerSnapshotObserver.observeReads$ui_release(OwnerSnapshotObserver.kt:133)
at androidx.compose.ui.node.OwnerSnapshotObserver.observeMeasureSnapshotReads$ui_release(OwnerSnapshotObserver.kt:113)

@xemniz xemniz added the bug Something isn't working label Nov 4, 2024
@marandaneto
Copy link
Member

Hello @xemniz thanks for the issue.
Are you using the screenshot or wireframe mode?
Which SDK version?
We don't call measure and layout at all but we do use draw here which should not be a problem anyway.
Can you provide a minimal reproducible example? I tried using a compose-sample app but did not succeed in reproducing the issue.

@marandaneto marandaneto added the question Further information is requested label Nov 5, 2024
@marandaneto marandaneto changed the title PostHogReplayThread Note that observation on multiple threads in layout/draw is not supported. Make sure your measure/layout/draw for each Owner (AndroidComposeView) is executed on the same thread Nov 5, 2024
@xemniz
Copy link
Author

xemniz commented Nov 5, 2024

image this commented lines made a difference, it was reproducible in all my screens, but randomly i'd say. in crashlytics crashfree sessions dropped to 65 percent with 3.9.0 version of posthog. image right here i checked thread names and the one that's not [main] was PostHogReplayThread and thet's the place where android framework does this check that throws an error

@xemniz
Copy link
Author

xemniz commented Nov 5, 2024

right here they discuss that you should call getAllSemanticNodes on ui thread, probably here is the problem

@marandaneto
Copy link
Member

@xemniz getAllSemanticsNodes is called within a try/catch, so IllegalArgumentException should not have crashed the app, looking into this though.

@marandaneto
Copy link
Member

@xemniz what's the version of your resolved androidx.compose.ui:ui?

@xemniz
Copy link
Author

xemniz commented Nov 5, 2024

androidx.compose:compose-bom:2024.10.00 - compose version

@xemniz
Copy link
Author

xemniz commented Nov 5, 2024

@xemniz getAllSemanticsNodes is called within a try/catch, so IllegalArgumentException should not have crashed the app, looking into this though.

yes, but having crash not in PostHogSdk code, but in androidx.compose.runtime.snapshots.SnapshotStateObserver, so try catch won't help

image

@marandaneto
Copy link
Member

Session replay tries to do as much as possible off of the main thread because of performance reasons, so the replay thread calls that function.
What we could do is:

            val latch = CountDownLatch(1)

            var semanticsNodes: List<SemanticsNode>? = null
            mainHandler.handler.post {
                try {
                    semanticsNodes = semanticsOwner.getAllSemanticsNodes(true)
                } catch (e: Throwable) {
                    config.logger.log("Session Replay getAllSemanticsNodes failed: $e")
                } finally {
                    latch.countDown()
                }
            }

            latch.await(1000, TimeUnit.MILLISECONDS)

So only that part will be accessed via the main thread, unless (view as? RootForTest)?.semanticsOwner, node.config, and node.boundsInWindow require main thread which I am trying to figure out.
I can't reproduce this issue yet so I can't verify that.

@marandaneto
Copy link
Member

@beradeep did you see this one, any ideas?

@beradeep
Copy link
Contributor

beradeep commented Nov 7, 2024

@marandaneto I looked into it just a bit, turns out I was able to reproduce the issue. But its occurrence is unpredictable. It came up a couple of times when I ran a sample app repeatedly for at least 10 times.

@marandaneto
Copy link
Member

@marandaneto I looked into it just a bit, turns out I was able to reproduce the issue. But its occurrence is unpredictable. It came up a couple of times when I ran a sample app repeatedly for at least 10 times.

Thanks, can you share the sample app you tried? So I can try to reproduce it myself as well.
Can you try if a solution like this would help?
Would be cool to at least narrow down which function call (if 1 or N) is causing this, maybe only getAllSemanticsNodes isn't enough.
Maybe this started after a specific compose version, etc.

Any help here is appreciated since I cannot reproduce the issue at all.

@beradeep
Copy link
Contributor

beradeep commented Nov 11, 2024

@marandaneto I faced the issue in the app startup, so I think it is practically possible with any compose app, although I only tried with androidx.compose:compose-bom:2024.10.00. I used this app.

The google issuetracker comment also mentions that they always call getAllSemanticNodes on the ui thread, as I later found here too. So I think this might be because we are calling it outside of the ui thread, and so this may solve the issue.

@beradeep
Copy link
Contributor

beradeep commented Nov 11, 2024

But as this comment mentions, the properties that we use i.e. node.boundsInWindow and node.config may not be thread safe too :(

@marandaneto
Copy link
Member

So in this case the whole block should be executed in the main thread.

@marandaneto
Copy link
Member

@beradeep #204
I think this should help, mind testing after release? thx.

@marandaneto
Copy link
Member

https://github.com/PostHog/posthog-android/releases/tag/3.9.1
Might take a few minutes until the version is available on maven central.
Let us know if that helps.

@beradeep
Copy link
Contributor

@beradeep #204 I think this should help, mind testing after release? thx.

Reran the same app for a number times on both 3.9.0 and 3.9.1. Crashed 5 times on the former, but none on the latter. LGTM!

@zhuziwei0408
Copy link

So in this case the whole block should be executed in the main thread.

The entire logic such as getAllSemanticsNodes is put into the main thread for execution. Have you ever encountered a situation where the execution is very slow and the main thread is stuck? I tested that getting SemanticsNodes in the main thread would cause a hang.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working question Further information is requested Session Replay
Projects
None yet
4 participants