Possible deadlock in persistToCoreData
#12061
Comments
|
Just kidding about the stack trace – TIL the LLDB command |
|
Moving up in priority, see internal reference: p77Llu-cdK-p2. |
|
Hey @danielebogo . I don't suppose you could take a look at this? |
|
@ScoutHarris I did already when I was working on the hotfix 12.7.1. I was able to replicate it many times. I made different tests and I can confirm it’s not CoreData related. I was able to make it happen removing all the code used to work on the CoreData stack. |
|
@designsimply @jkmassel I merged today #12169 where the Insight store has been improved, removing the reference to the Period store. This means we won't have the double reload mentioned above. I wasn't able to reproduce this crash with this fix so I would like to keep this issue open and see how 13.0 will behave. |
|
@danielebogo I tried launching the app in the simulator as was described in the ticket and it didn't crash. I checked for |
|
@designsimply Usually at that time I opened Stats -> Insights and navigated back while the store was still loading. Then opened Insights again and press back. I was doing this action till it crashed. But after the fix I wasn't able to reproduce it. |
|
Ace. I tried the steps you mentioned and also couldn't reproduce the crash. Testing steps:
Result: I couldn't replicate a crash using the testing steps above. |
|
@danielebogo do you think it's okay to close this one now or do you feel we need a bit more testing? |
|
@designsimply I think we can close this for now and see if we are going to see a similar one with the next releases. We are also improving the Stats loading atm. Thanks for testing this! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I came across this crash in the simulator while launching the app:
Unfortunately I don't have a full stack trace, but I did have the debugger attached.
It seems we're somehow causing the stats store to refresh twice at the same time – both Thread 3 and 4 are in the
statsInsightStore.persistToCoreData()method. It looks like there's some sort of lock in the implementation, and both threads are taking it.The actual crash itself happens in Thread 18:
HTTPProtocol::shouldAttemptOriginLoad()is failing withEXC_BAD_ACCESS. Lower down in the stack is the call fromStatsInsightsStore(andStatsPeriodStore), which I suspect has something to do with the double-reloading above.I've seen similar bugs in Sentry, but not this exact one. I suspect this issue is happening in production as well – possibly as part of stats state restoration on launch?
A few thoughts while writing this that may or may not be helpful:
Assuming these insights are computed and need to be cached using
persistToCoreData(), perhaps this cache write should follow the computation rather than preceding a lookup?It appears that
persistToCoreData()is entirely synchronous. That means it could be made thread-safe with a serialized queue.I'm not sure how easy it would be to detect whether or not there have been state changes since the most recent call to
persistToCoreData(), but if it were possible to only proceed if the store was dirty, that might help resolve this as well, and prevent unnecessary writes to the device.The text was updated successfully, but these errors were encountered: