Fix threading issue in IPC connection #210
Conversation
This change addresses the possibility of `DTXIPCConnection._slaveDidConnectWithName` being called from non-main thread by dispatching the work to main thread when it is being called from non main thread. This is necessary so that `_otherConnection` gets assigned on main thread. `SBTUITestTunnelClient.serverDidConnect` is updated to wait for `ipcConnection` to become valid. This is needed because `DTXIPCConnection._slaveDidConnectWithName` and `SBTUITestTunnelClient.serverDidConnect` can be called from different threads upon which we dispatch both to main. But `SBTUITestTunnelClient.serverDidConnect` work can start first at which point it can lead to assertion of `ipcConnection` not being valid as `ipcConnection._otherConnection` has not yet been assigned.
alvar-bolt
force-pushed
the
alvar/theading-fix-issue
branch
from
360d53c
to
5e72532
Compare
|
Nice! I'll take a better look at the changes next week. |
|
Apologies for the long delay! I have reviewed the state flow during the initialization process, and I agree that there are potential race conditions as you pointed out. Below, I have summarized the initial phases of the connection just for convenience Instead of recursively calling |
|
Hey. I agree, synchronising it on separate queue is better approach. I tried your solution on our test suite and did not get any assertions from ~3000 runs. So I'm happy to continue with your solution. |
|
Thanks a lot for giving it a try! I opened #212 and will try to merge it soon. Apologies again for the slow reaction time, but it's a particularly busy period right now! |
|
No need to apologise. You are offering a free software and it's not an easy task to maintain open source project. So, I'm very thankful to you for even taking time and looking into it. |
|
I've merged #212, gonna close this one. Thanks again for bringing up this issue 👍 |
This change addresses the possibility of
DTXIPCConnection._slaveDidConnectWithNamebeing called from non-main thread by dispatching the work to main thread when it is being called from non main thread. This is necessary so that_otherConnectiongets assigned on main thread.SBTUITestTunnelClient.serverDidConnectis updated to wait foripcConnectionto become valid. This is needed becauseDTXIPCConnection._slaveDidConnectWithNameandSBTUITestTunnelClient.serverDidConnectcan be called from different threads upon which we dispatch both to main. ButSBTUITestTunnelClient.serverDidConnectwork can start first at which point it can lead to assertion ofipcConnectionnot being valid asipcConnection._otherConnectionhas not yet been assigned.These threading issues can result in following assertion:
In order to debug this issue, I added bunch of NSLogs to the codebase (alvar-bolt@e96bbc4#diff-313307a5a8b84223b5eb7add983e43794ab1c14ea722485ed889152e5519b725R326).
From there, I ran a test that called
app.userDefaultsObjectimmediately after tunnel was launched successfully. This is important as it ends up calling-[_DTXIPCDistantObject forwardInvocation:]on main thread which in turn calls_connection.isValidwhich is:Important part of that code is access of
_otherConnection. As current bug allows writing of it from different thread, then we cannot be sure that it is set.In our codebase is was able to find a test case where
_slaveDidConnectWithNamewas called on non main thread in ~25% of the cases. Luckily I was also able to observe that behaviour inSBTUITestTunnel_Tests.MiscellaneousTests.testCustomCommand(), although there it happens in ~1% of the cases.This behaviour does not always lead to assertion as there is still possibility that
_otherConnectiongets stored before it is being accessed from main thread. But if you are unlucky enough, it is still in progress and so, theisValidcheck will fail.Example of _slaveDidConnectWithName