Fix pthread_mutex_t dangling pointers #111
Conversation
digitaliz
force-pushed
the
fix-pthread_mutex_t-dangling-pointer
branch
from
0949384
to
ca1a7a2
Compare
| func unlock() { | ||
| pthread_mutex_unlock(self) | ||
| } | ||
|
|
||
| /// Will lock `self`, call `block`, then unlock `self` |
There was a problem hiding this comment.
I removed protect because it triggered Thread 1: Simultaneous accesses to 0x1032d57a8, but modification requires exclusive access when running the FlowTests. I currently don't have a good enough mental model to understand why that is (but a guess is recursively calling protect, starts two possible mutating scopes). Since I was unable to clearly reason about this construct, and for now can't do more research, I decided to remove it in favour of straight mutex lock/unlock that I understand better.
Unclear to me if it's supported
digitaliz
force-pushed
the
fix-pthread_mutex_t-dangling-pointer
branch
from
4c49587
to
f32c157
Compare
| @@ -41,9 +41,11 @@ public final class FutureQueue<Resource> { | |||
| queueScheduler = executeOn | |||
| OSAtomicIncrement32(&futureQueueUnitTestAliveCount) | |||
| memPrint("Queue init", futureQueueUnitTestAliveCount) | |||
| mutex.initialize(as: .recursive) | |||
There was a problem hiding this comment.
Before adding the ability to initialize this as a recursive mutex, it deadlocked in FlowTests.FutureQueueTests testBatchQueue.
|
Hey, I think it's possible we've encountered this issue before; We've been seeing stack traces where it looks like we crash trying to use this nonatomic dangling mutex pointer 💀 I'm far from an expert on it, but from my understanding it seems to fit with your & The Eskimo's explanation of what's going on here. |
|
Is everyone happy to merge this? Would be great to have his fix available to use |
|
Hey @alasdairlaw we are sing some issues with this update on the Go apps side. Looking in to it for how to work around them. |
|
@moglistree We should open up an issue/PR for the problems we see with UITests in Go project. I'll write a comment here for now to just capture thoughts/where we are right now. I've just done bit of investigation on the failing UITests, my results are that they are all happening in edit mode for ProductDetailView, ie the view for adding a product to the Product Library. I still need to verify but seem connected to testing of input fields in that view. Specifically for fields presenting a keyboard I think it could be that at the point of dismissal/relinquish main responder, main thread is deadlocked. The abstraction distance between this change and failing UITests in Go app is quite long. My suggestion is that I should pair with someone that has experience with the product detail view and it's UITests, we can possibly figure it out much faster together? I'll ping @achernenko-pp here to get his thoughts :) |
|
Follow-up thought for a possible action, for the Flow test suite to pass I had to enable recursive locks in FutureQueue, #111 (comment). The idea I have now is that the failing test in PL UITests is because the same type of issue, there is re-entrant locking going on but the lock is not enabled as recursive. My thinking is that this could be something that is not captured by the Flow tests suite. So for a next action I'm going to see if making Flow locks recursive by default makes the PL UITests pass. |
|
CircleCI is down atm. but locally Flow test suite passes after making recursive locks the default. Running PL UITests locally after fastlane update frameworks, it seems making recursive locks the default does not make the PL UITests pass. I'd like to verify with @moglistree it's actually running the version with default recursive locks before conclusively closing that investigation. An activity I think could be next up is to manually execute the UITests that are failing, if failing behaviour is experienced then we can rule out that the UI testing code/framework itself is involved in the failure. |
Fixes #110
There is some background reasoning on the original issue #110 (comment)