(#1322) Fix crashes in test case PushTests.test__008__LocalDevice__has_a_device_method_that_returns_a_LocalDevice (4f837671-6233-41f1-94e8-01174d1da7b8)
#1388
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lawrence-forooghian
added
bug
lawrence-forooghian
force-pushed
the
1322-fix-test-case-4f837671-6233-41f1-94e8-01174d1da7b8-take-3
branch
from
5aeb990
to
e19e037
Compare
We currently read / write the variable that stores the shared ARTLocalDevice variable from the internal queue of the ARTRest instance through which the device is being accessed. But, since each ARTRest instance has its own queue, this means that access to this variable isn’t currently synchronised between multiple ARTRest instances. So, here we do that. This also allows us to remove our usage of dispatch_once. This is a good thing, because we were using it incorrectly before. From an Apple engineer on https://stackoverflow.com/a/19845164: > The implementation of dispatch_once() requires that the > dispatch_once_t is zero, and has never been non-zero. That is, you can’t reset a dispatch_once_t variable and expect dispatch_once to still work with it, which is what we were doing.
This doesn’t change anything, it just explains the current behaviour of the code. ARTLocalDeviceStorage already conforms to these requirements, since it uses the (already thread-safe) user defaults / keychain. However, MockDeviceStorage does need some synchronisation. I’ll do that next.
As promised in e0ff029. Together with de281bd and fbd5645, I believe this closes #1322 (various crashes in PushTests.test__008__LocalDevice__has_a_device_method_that_returns_a_LocalDevice, which seemed to be related to threading). Test case: https://test-observability.herokuapp.com/repos/ably/ably-cocoa/test_cases/4f837671-6233-41f1-94e8-01174d1da7b8
lawrence-forooghian
force-pushed
the
1322-fix-test-case-4f837671-6233-41f1-94e8-01174d1da7b8-take-3
branch
from
e19e037
to
280a236
Compare
lawrence-forooghian
requested review from
lukasz-szyszkowski,
maratal and
QuintinWillison
|
D'oh, put the wrong issue number. Gonna open a new PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this do?
Introduces various concurrency-related fixes related to the local device. Together, I believe these fix various crashes that we've observed in the test mentioned in the title (#1322). See commit messages for more details.
How do I know it's fixed the test?
These are the results of the tests running continuously with these fixes applied. At time of writing, there are 97 test runs with no failures in this test, but I'll keep it running for a while more before merging.