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

SCNetworkReachabilitySetCallback (Crashed: io.ably.main) #1380

Closed
MrNiceMrGood opened this issue Apr 29, 2022 · 1 comment · Fixed by #1453 or #1565
Closed

SCNetworkReachabilitySetCallback (Crashed: io.ably.main) #1380

MrNiceMrGood opened this issue Apr 29, 2022 · 1 comment · Fixed by #1453 or #1565
Assignees
Labels
bug Something isn't working. It's clear that this does need to be fixed.

Comments

@MrNiceMrGood
Copy link

MrNiceMrGood commented Apr 29, 2022

Which version of the Ably SDK are you using?

1.2.10

On which platform does the issue happen?

Multiple

Are you using Cocoapods?

1.11.2

Which version of Xcode are you using?

Xcode 13.3.1
Build version 13E500a

What did you do?

I don't have too much info. I have found this to happen to some users in production. Never managed to reproduce directly.
You can check the firebase stack trace.

This looks like a similar issue as #593.

Crashed: io.ably.main
0  libsystem_pthread.dylib        0x4924 <redacted> + 126
1  SystemConfiguration            0x28938 SCNetworkReachabilitySetCallback + 64
2  Ably                           0x27570 -[ARTOSReachability listenForHost:callback:] + 348
3  Ably                           0x3df10 -[ARTRealtimeInternal transitionSideEffects:] + 4516
4  Ably                           0x3ca90 -[ARTRealtimeInternal transition:withErrorInfo:] + 388
5  Ably                           0x17210 -[ARTEventListener timeout] + 84
6  Ably                           0x1738c __30-[ARTEventListener startTimer]_block_invoke + 36
7  Ably                           0x1ae88 __53-[ARTScheduledBlockHandle initWithDelay:queue:block:]_block_invoke + 100
8  libdispatch.dylib              0x103e4 _dispatch_block_async_invoke2 + 104
9  libdispatch.dylib              0x61298 _dispatch_client_callout + 16
10 libdispatch.dylib              0x6028 _dispatch_continuation_pop$VARIANT$mp + 412
11 libdispatch.dylib              0x16d64 _dispatch_source_invoke$VARIANT$mp + 1304
12 libdispatch.dylib              0x9908 _dispatch_lane_serial_drain$VARIANT$mp + 300
13 libdispatch.dylib              0xa518 _dispatch_lane_invoke$VARIANT$mp + 420
14 libdispatch.dylib              0x13fac _dispatch_workloop_worker_thread + 712
15 libsystem_pthread.dylib        0xb5bc _pthread_wqthread + 272
16 libsystem_pthread.dylib        0xe86c start_wqthread + 8

┆Issue is synchronized with this Jira Bug by Unito

@ikbalkaya ikbalkaya added the bug Something isn't working. It's clear that this does need to be fixed. label Jul 28, 2022
@maratal maratal self-assigned this Jul 30, 2022
@maratal
Copy link
Collaborator

maratal commented Dec 14, 2022

Reopening this as well since #593 was reopened.

@maratal maratal reopened this Dec 14, 2022
lawrence-forooghian added a commit to ably/ably-asset-tracking-swift that referenced this issue Mar 15, 2023
Based on Android at f6d163f

TODO explain why it's not using async await (also I have no idea how to
do timeouts)

differences:

- always close susbcriber monitor’s Ably so that it's closed if error
  and so we don't forget to do it (like Android does someitmes)

TODO there is a crash seemingly due to the bad access in ably-cocoa
reachability

TODO explain re ably-cocoa:

I was using

"branch" : "fix/1380-dispatch-ARTOSReachability_Callback",
"revision" : "036be28852b3ef1b3a112aea7052c463c5b8792a"

because of reachability crashes related to
ably/ably-cocoa#1380

internal thread:

https://ably-real-time.slack.com/archives/CSQEKCE81/p1678802535684519

but it appears (although perhaps it was more sporadic than I imagined)
all of the affected tests fail anyway, so now that I’ve marked those
tests as skipped, I’ve reverted that, and we can deal with it when we
try re-enabling those tests in
#575, by which
point hopefully the fix for ably-cocoa#1380 will have been released.

(I don't want AAT to use an ably-cocoa branch.)
lawrence-forooghian added a commit to ably/ably-asset-tracking-swift that referenced this issue Mar 15, 2023
Based on Android at f6d163f

TODO explain why it's not using async await (also I have no idea how to
do timeouts)

differences:

- always close susbcriber monitor’s Ably so that it's closed if error
  and so we don't forget to do it (like Android does someitmes)

TODO there is a crash seemingly due to the bad access in ably-cocoa
reachability

TODO explain re ably-cocoa:

I was using

"branch" : "fix/1380-dispatch-ARTOSReachability_Callback",
"revision" : "036be28852b3ef1b3a112aea7052c463c5b8792a"

because of reachability crashes related to
ably/ably-cocoa#1380

internal thread:

https://ably-real-time.slack.com/archives/CSQEKCE81/p1678802535684519

but it appears (although perhaps it was more sporadic than I imagined)
all of the affected tests fail anyway, so now that I’ve marked those
tests as skipped, I’ve reverted that, and we can deal with it when we
try re-enabling those tests in
#575, by which
point hopefully the fix for ably-cocoa#1380 will have been released.

(I don't want AAT to use an ably-cocoa branch.)

TODO check docstrings
lawrence-forooghian added a commit to ably/ably-asset-tracking-swift that referenced this issue Mar 16, 2023
These are based on the corresponding tests in the Android codebase at
commit f6d163f. They stick as closely as possible to the structure and
behaviour of those tests, to which I have not applied much of a critical
eye. The majority of the log messages and comments are simply copied
across.

The only structural difference between these tests and the Android ones
is that I have removed the caller’s responsibility to call
`SubscriberMonitor#close`. There are places in the Android
implementation where we forget to call it, and also it isn’t called if
an error is thrown by `waitForStateTransition`. By making the subscriber
monitor responsible for its own cleanup, we remove these issues.

When I started writing these tests, we were still targeting iOS 12 and
above, which meant it was not possible to use Swift concurrency. Hence,
all of the code is written using either completion handlers or blocking
functions. Since then, we’ve decided to set our deployment target to iOS
13 (see #597), which means we could now use Swift concurrency.  However,
by that point I was well into the writing of these tests and did not
want to re-structure them, especially since they require functionality
that doesn’t exist out of the box in Swift concurrency (for example
timeouts). We may wish to revisit at some point and switch to using
Swift concurrency, but I don’t think it’s urgent.

In order to generate the list of which faults to skip, I ran all of the
tests and checked which ones failed. Whilst doing so, I encountered a
crash in ably-cocoa, which I believe is already reported as
ably/ably-cocoa#1380. In order to get past this issue, I tried using
Marat’s fix from ably-cocoa branch
`fix/1380-dispatch-ARTOSReachability_Callback` (commit 036be28). This
removed the crashes. The list of skipped tests is the list of tests that
failed when using this branch. It then actually turned out that the
non-skipped tests do not exhibit this crash anyway, so I didn’t include
any version change to ably-cocoa in this PR. We can deal with this crash
when we try re-enabling the tests in #575, by which point hopefully the
fix for ably-cocoa#1380 will have been released. (Internal thread re
this crash and its appearance in these tests is [1].)

TODO check copied docstrings
lawrence-forooghian added a commit to ably/ably-asset-tracking-swift that referenced this issue Mar 16, 2023
Resolves #539.

These are based on the corresponding tests in the Android codebase at
commit f6d163f. They stick as closely as possible to the structure and
behaviour of those tests, to which I have not applied much of a critical
eye. The majority of the log messages and comments are simply copied
across.

The only structural difference between these tests and the Android ones
is that I have removed the caller’s responsibility to call
`SubscriberMonitor#close`. There are places in the Android
implementation where we forget to call it, and also it isn’t called if
an error is thrown by `waitForStateTransition`. By making the subscriber
monitor responsible for its own cleanup, we remove these issues.

When I started writing these tests, we were still targeting iOS 12 and
above, which meant it was not possible to use Swift concurrency. Hence,
all of the code is written using either completion handlers or blocking
functions. Since then, we’ve decided to set our deployment target to iOS
13 (see #597), which means we could now use Swift concurrency.  However,
by that point I was well into the writing of these tests and did not
want to re-structure them, especially since they require functionality
that doesn’t exist out of the box in Swift concurrency (for example
timeouts). We may wish to revisit at some point and switch to using
Swift concurrency, but I don’t think it’s urgent.

In order to generate the list of which faults to skip, I ran all of the
tests and checked which ones failed. Whilst doing so, I encountered a
crash in ably-cocoa, which I believe is already reported as
ably/ably-cocoa#1380. In order to get past this issue, I tried using
Marat’s fix from ably-cocoa branch
`fix/1380-dispatch-ARTOSReachability_Callback` (commit 036be28). This
removed the crashes. The list of skipped tests is the list of tests that
failed when using this branch. It then actually turned out that the
non-skipped tests do not exhibit this crash anyway, so I didn’t include
any version change to ably-cocoa in this PR. We can deal with this crash
when we try re-enabling the tests in #575, by which point hopefully the
fix for ably-cocoa#1380 will have been released. (Internal thread re
this crash and its appearance in these tests is [1].)

TODO check copied docstrings
lawrence-forooghian added a commit to ably/ably-asset-tracking-swift that referenced this issue Mar 16, 2023
Resolves #539.

These are based on the corresponding tests in the Android codebase at
commit f6d163f. They stick as closely as possible to the structure and
behaviour of those tests, to which I have not applied much of a critical
eye. The majority of the log messages and comments are simply copied
across.

The only structural difference between these tests and the Android ones
is that I have removed the caller’s responsibility to call
`SubscriberMonitor#close`. There are places in the Android
implementation where we forget to call it, and also it isn’t called if
an error is thrown by `waitForStateTransition`. By making the subscriber
monitor responsible for its own cleanup, we remove these issues.

When I started writing these tests, we were still targeting iOS 12 and
above, which meant it was not possible to use Swift concurrency. Hence,
all of the code is written using either completion handlers or blocking
functions. Since then, we’ve decided to set our deployment target to iOS
13 (see #597), which means we could now use Swift concurrency.  However,
by that point I was well into the writing of these tests and did not
want to re-structure them, especially since they require functionality
that doesn’t exist out of the box in Swift concurrency (for example
timeouts). We may wish to revisit at some point and switch to using
Swift concurrency, but I don’t think it’s urgent.

In order to generate the list of which faults to skip, I ran all of the
tests and checked which ones failed. Whilst doing so, I encountered a
crash in ably-cocoa, which I believe is already reported as
ably/ably-cocoa#1380. In order to get past this issue, I tried using
Marat’s fix from ably-cocoa branch
`fix/1380-dispatch-ARTOSReachability_Callback` (commit 036be28). This
removed the crashes. The list of skipped tests is the list of tests that
failed when using this branch. It then actually turned out that the
non-skipped tests do not exhibit this crash anyway, so I didn’t include
any version change to ably-cocoa in this PR. We can deal with this crash
when we try re-enabling the tests in #575, by which point hopefully the
fix for ably-cocoa#1380 will have been released. (Internal thread re
this crash and its appearance in these tests is [1].)
lawrence-forooghian added a commit to ably/ably-asset-tracking-swift that referenced this issue Mar 17, 2023
Resolves #539.

These are based on the corresponding tests in the Android codebase at
commit f6d163f. They stick as closely as possible to the structure and
behaviour of those tests, to which I have not applied much of a critical
eye. The majority of the log messages and comments are simply copied
across.

The only structural difference between these tests and the Android ones
is that I have removed the caller’s responsibility to call
`SubscriberMonitor#close`. There are places in the Android
implementation where we forget to call it, and also it isn’t called if
an error is thrown by `waitForStateTransition`. By making the subscriber
monitor responsible for its own cleanup, we remove these issues.

When I started writing these tests, we were still targeting iOS 12 and
above, which meant it was not possible to use Swift concurrency. Hence,
all of the code is written using either completion handlers or blocking
functions. Since then, we’ve decided to set our deployment target to iOS
13 (see #597), which means we could now use Swift concurrency.  However,
by that point I was well into the writing of these tests and did not
want to re-structure them, especially since they require functionality
that doesn’t exist out of the box in Swift concurrency (for example
timeouts). We may wish to revisit at some point and switch to using
Swift concurrency, but I don’t think it’s urgent.

In order to generate the list of which faults to skip, I ran all of the
tests and checked which ones failed. Whilst doing so, I encountered a
crash in ably-cocoa, which I believe is already reported as
ably/ably-cocoa#1380. In order to get past this issue, I tried using
Marat’s fix from ably-cocoa branch
`fix/1380-dispatch-ARTOSReachability_Callback` (commit 036be28). This
removed the crashes. The list of skipped tests is the list of tests that
failed when using this branch. It then actually turned out that the
non-skipped tests do not exhibit this crash anyway, so I didn’t include
any version change to ably-cocoa in this PR. We can deal with this crash
when we try re-enabling the tests in #575, by which point hopefully the
fix for ably-cocoa#1380 will have been released. (Internal thread re
this crash and its appearance in these tests is [1].)
lawrence-forooghian added a commit to ably/ably-asset-tracking-swift that referenced this issue Mar 21, 2023
Resolves #539.

These are based on the corresponding tests in the Android codebase at
commit f6d163f. They stick as closely as possible to the structure and
behaviour of those tests, to which I have not applied much of a critical
eye. The majority of the log messages and comments are simply copied
across.

The only structural difference between these tests and the Android ones
is that I have removed the caller’s responsibility to call
`SubscriberMonitor#close`. There are places in the Android
implementation where we forget to call it, and also it isn’t called if
an error is thrown by `waitForStateTransition`. By making the subscriber
monitor responsible for its own cleanup, we remove these issues.

When I started writing these tests, we were still targeting iOS 12 and
above, which meant it was not possible to use Swift concurrency. Hence,
all of the code is written using either completion handlers or blocking
functions. Since then, we’ve decided to set our deployment target to iOS
13 (see #597), which means we could now use Swift concurrency.  However,
by that point I was well into the writing of these tests and did not
want to re-structure them, especially since they require functionality
that doesn’t exist out of the box in Swift concurrency (for example
timeouts). We may wish to revisit at some point and switch to using
Swift concurrency, but I don’t think it’s urgent.

In order to generate the list of which faults to skip, I ran all of the
tests and checked which ones failed. Whilst doing so, I encountered a
crash in ably-cocoa, which I believe is already reported as
ably/ably-cocoa#1380. In order to get past this issue, I tried using
Marat’s fix from ably-cocoa branch
`fix/1380-dispatch-ARTOSReachability_Callback` (commit 036be28). This
removed the crashes. The list of skipped tests is the list of tests that
failed when using this branch. It then actually turned out that the
non-skipped tests do not exhibit this crash anyway, so I didn’t include
any version change to ably-cocoa in this PR. We can deal with this crash
when we try re-enabling the tests in #575, by which point hopefully the
fix for ably-cocoa#1380 will have been released. (Internal thread re
this crash and its appearance in these tests is [1].)
lawrence-forooghian added a commit to ably/ably-asset-tracking-swift that referenced this issue Mar 21, 2023
Resolves #539.

These are based on the corresponding tests in the Android codebase at
commit f6d163f. They stick as closely as possible to the structure and
behaviour of those tests, to which I have not applied much of a critical
eye. The majority of the log messages and comments are simply copied
across.

The only structural difference between these tests and the Android ones
is that I have removed the caller’s responsibility to call
`SubscriberMonitor#close`. There are places in the Android
implementation where we forget to call it, and also it isn’t called if
an error is thrown by `waitForStateTransition`. By making the subscriber
monitor responsible for its own cleanup, we remove these issues.

When I started writing these tests, we were still targeting iOS 12 and
above, which meant it was not possible to use Swift concurrency. Hence,
all of the code is written using either completion handlers or blocking
functions. Since then, we’ve decided to set our deployment target to iOS
13 (see #597), which means we could now use Swift concurrency.  However,
by that point I was well into the writing of these tests and did not
want to re-structure them, especially since they require functionality
that doesn’t exist out of the box in Swift concurrency (for example
timeouts). We may wish to revisit at some point and switch to using
Swift concurrency, but I don’t think it’s urgent.

In order to generate the list of which faults to skip, I ran all of the
tests and checked which ones failed. Whilst doing so, I encountered a
crash in ably-cocoa, which I believe is already reported as
ably/ably-cocoa#1380. In order to get past this issue, I tried using
Marat’s fix from ably-cocoa branch
`fix/1380-dispatch-ARTOSReachability_Callback` (commit 036be28). This
removed the crashes. The list of skipped tests is the list of tests that
failed when using this branch. It then actually turned out that the
non-skipped tests do not exhibit this crash anyway, so I didn’t include
any version change to ably-cocoa in this PR. We can deal with this crash
when we try re-enabling the tests in #575, by which point hopefully the
fix for ably-cocoa#1380 will have been released. (Internal thread re
this crash and its appearance in these tests is [1].)

[1] https://ably-real-time.slack.com/archives/CSQEKCE81/p1678802535684519
lawrence-forooghian added a commit to ably/ably-asset-tracking-swift that referenced this issue Mar 21, 2023
Resolves #539.

These are based on the corresponding tests in the Android codebase at
commit f6d163f. They stick as closely as possible to the structure and
behaviour of those tests, to which I have not applied much of a critical
eye. The majority of the log messages and comments are simply copied
across.

The only structural difference between these tests and the Android ones
is that I have removed the caller’s responsibility to call
`SubscriberMonitor#close`. There are places in the Android
implementation where we forget to call it, and also it isn’t called if
an error is thrown by `waitForStateTransition`. By making the subscriber
monitor responsible for its own cleanup, we remove these issues.

When I started writing these tests, we were still targeting iOS 12 and
above, which meant it was not possible to use Swift concurrency. Hence,
all of the code is written using either completion handlers or blocking
functions. Since then, we’ve decided to set our deployment target to iOS
13 (see #597), which means we could now use Swift concurrency.  However,
by that point I was well into the writing of these tests and did not
want to re-structure them, especially since they require functionality
that doesn’t exist out of the box in Swift concurrency (for example
timeouts). We may wish to revisit at some point and switch to using
Swift concurrency, but I don’t think it’s urgent.

In order to generate the list of which faults to skip, I ran all of the
tests and checked which ones failed. Whilst doing so, I encountered a
crash in ably-cocoa, which I believe is already reported as
ably/ably-cocoa#1380. In order to get past this issue, I tried using
Marat’s fix from ably-cocoa branch
`fix/1380-dispatch-ARTOSReachability_Callback` (commit 036be28). This
removed the crashes. The list of skipped tests is the list of tests that
failed when using this branch. It then actually turned out that the
non-skipped tests do not exhibit this crash anyway, so I didn’t include
any version change to ably-cocoa in this PR. We can deal with this crash
when we try re-enabling the tests in #575, by which point hopefully the
fix for ably-cocoa#1380 will have been released. (Internal thread re
this crash and its appearance in these tests is [1].)

[1] https://ably-real-time.slack.com/archives/CSQEKCE81/p1678802535684519
lawrence-forooghian added a commit to ably/ably-asset-tracking-swift that referenced this issue Mar 22, 2023
Resolves #539.

These are based on the corresponding tests in the Android codebase at
commit f6d163f. They stick as closely as possible to the structure and
behaviour of those tests, to which I have not applied much of a critical
eye. The majority of the log messages and comments are simply copied
across.

The only structural difference between these tests and the Android ones
is that I have removed the caller’s responsibility to call
`SubscriberMonitor#close`. There are places in the Android
implementation where we forget to call it, and also it isn’t called if
an error is thrown by `waitForStateTransition`. By making the subscriber
monitor responsible for its own cleanup, we remove these issues.

When I started writing these tests, we were still targeting iOS 12 and
above, which meant it was not possible to use Swift concurrency. Hence,
all of the code is written using either completion handlers or blocking
functions. Since then, we’ve decided to set our deployment target to iOS
13 (see #597), which means we could now use Swift concurrency.  However,
by that point I was well into the writing of these tests and did not
want to re-structure them, especially since they require functionality
that doesn’t exist out of the box in Swift concurrency (for example
timeouts). We may wish to revisit at some point and switch to using
Swift concurrency, but I don’t think it’s urgent.

In order to generate the list of which faults to skip, I ran all of the
tests and checked which ones failed. Whilst doing so, I encountered a
crash in ably-cocoa, which I believe is already reported as
ably/ably-cocoa#1380. In order to get past this issue, I tried using
Marat’s fix from ably-cocoa branch
`fix/1380-dispatch-ARTOSReachability_Callback` (commit 036be28). This
removed the crashes. The list of skipped tests is the list of tests that
failed when using this branch. It then actually turned out that the
non-skipped tests do not exhibit this crash anyway, so I didn’t include
any version change to ably-cocoa in this PR. We can deal with this crash
when we try re-enabling the tests in #575, by which point hopefully the
fix for ably-cocoa#1380 will have been released. (Internal thread re
this crash and its appearance in these tests is [1].)

[1] https://ably-real-time.slack.com/archives/CSQEKCE81/p1678802535684519
lawrence-forooghian added a commit to ably/ably-asset-tracking-swift that referenced this issue Mar 22, 2023
Resolves #539.

These are based on the corresponding tests in the Android codebase at
commit f6d163f (plus a fix added in eea952b). They stick as closely as
possible to the structure and behaviour of those tests, to which I have
not applied much of a critical eye. The majority of the log messages and
comments are simply copied across.

The only structural difference between these tests and the Android ones
is that I have removed the caller’s responsibility to call
`SubscriberMonitor#close`. There are places in the Android
implementation where we forget to call it, and also it isn’t called if
an error is thrown by `waitForStateTransition`. By making the subscriber
monitor responsible for its own cleanup, we remove these issues.

When I started writing these tests, we were still targeting iOS 12 and
above, which meant it was not possible to use Swift concurrency. Hence,
all of the code is written using either completion handlers or blocking
functions. Since then, we’ve decided to set our deployment target to iOS
13 (see #597), which means we could now use Swift concurrency.  However,
by that point I was well into the writing of these tests and did not
want to re-structure them, especially since they require functionality
that doesn’t exist out of the box in Swift concurrency (for example
timeouts). We may wish to revisit at some point and switch to using
Swift concurrency, but I don’t think it’s urgent.

In order to generate the list of which faults to skip, I ran all of the
tests and checked which ones failed. Whilst doing so, I encountered a
crash in ably-cocoa, which I believe is already reported as
ably/ably-cocoa#1380. In order to get past this issue, I tried using
Marat’s fix from ably-cocoa branch
`fix/1380-dispatch-ARTOSReachability_Callback` (commit 036be28). This
removed the crashes. The list of skipped tests is the list of tests that
failed when using this branch. It then actually turned out that the
non-skipped tests do not exhibit this crash anyway, so I didn’t include
any version change to ably-cocoa in this PR. We can deal with this crash
when we try re-enabling the tests in #575, by which point hopefully the
fix for ably-cocoa#1380 will have been released. (Internal thread re
this crash and its appearance in these tests is [1].)

[1] https://ably-real-time.slack.com/archives/CSQEKCE81/p1678802535684519
lawrence-forooghian added a commit to ably/ably-asset-tracking-swift that referenced this issue Mar 22, 2023
Resolves #539.

These are based on the corresponding tests in the Android codebase at
commit f6d163f (plus a fix added in eea952b). They stick as closely as
possible to the structure and behaviour of those tests, to which I have
not applied much of a critical eye. The majority of the log messages and
comments are simply copied across.

The only structural difference between these tests and the Android ones
is that I have removed the caller’s responsibility to call
`SubscriberMonitor#close`. There are places in the Android
implementation where we forget to call it, and also it isn’t called if
an error is thrown by `waitForStateTransition`. By making the subscriber
monitor responsible for its own cleanup, we remove these issues.

When I started writing these tests, we were still targeting iOS 12 and
above, which meant it was not possible to use Swift concurrency. Hence,
all of the code is written using either completion handlers or blocking
functions. Since then, we’ve decided to set our deployment target to iOS
13 (see #597), which means we could now use Swift concurrency.  However,
by that point I was well into the writing of these tests and did not
want to re-structure them, especially since they require functionality
that doesn’t exist out of the box in Swift concurrency (for example
timeouts). We may wish to revisit at some point and switch to using
Swift concurrency, but I don’t think it’s urgent.

In order to generate the list of which faults to skip, I ran all of the
tests and checked which ones failed. Whilst doing so, I encountered a
crash in ably-cocoa, which I believe is already reported as
ably/ably-cocoa#1380. In order to get past this issue, I tried using
Marat’s fix from ably-cocoa branch
`fix/1380-dispatch-ARTOSReachability_Callback` (commit 036be28). This
removed the crashes. The list of skipped tests is the list of tests that
failed when using this branch. It then actually turned out that the
non-skipped tests do not exhibit this crash anyway, so I didn’t include
any version change to ably-cocoa in this PR. We can deal with this crash
when we try re-enabling the tests in #575, by which point hopefully the
fix for ably-cocoa#1380 will have been released. (Internal thread re
this crash and its appearance in these tests is [1].)

[1] https://ably-real-time.slack.com/archives/CSQEKCE81/p1678802535684519
lawrence-forooghian added a commit to ably/ably-asset-tracking-swift that referenced this issue Mar 27, 2023
Resolves #539.

These are based on the corresponding tests in the Android codebase at
commit f6d163f (plus a fix added in eea952b). They stick as closely as
possible to the structure and behaviour of those tests, to which I have
not applied much of a critical eye. The majority of the log messages and
comments are simply copied across.

The only structural difference between these tests and the Android ones
is that I have removed the caller’s responsibility to call
`SubscriberMonitor#close`. There are places in the Android
implementation where we forget to call it, and also it isn’t called if
an error is thrown by `waitForStateTransition`. By making the subscriber
monitor responsible for its own cleanup, we remove these issues.

When I started writing these tests, we were still targeting iOS 12 and
above, which meant it was not possible to use Swift concurrency. Hence,
all of the code is written using either completion handlers or blocking
functions. Since then, we’ve decided to set our deployment target to iOS
13 (see #597), which means we could now use Swift concurrency.  However,
by that point I was well into the writing of these tests and did not
want to re-structure them, especially since they require functionality
that doesn’t exist out of the box in Swift concurrency (for example
timeouts). We may wish to revisit at some point and switch to using
Swift concurrency, but I don’t think it’s urgent.

In order to generate the list of which faults to skip, I ran all of the
tests and checked which ones failed. Whilst doing so, I encountered a
crash in ably-cocoa, which I believe is already reported as
ably/ably-cocoa#1380. In order to get past this issue, I tried using
Marat’s fix from ably-cocoa branch
`fix/1380-dispatch-ARTOSReachability_Callback` (commit 036be28). This
removed the crashes. The list of skipped tests is the list of tests that
failed when using this branch. It then actually turned out that the
non-skipped tests do not exhibit this crash anyway, so I didn’t include
any version change to ably-cocoa in this PR. We can deal with this crash
when we try re-enabling the tests in #575, by which point hopefully the
fix for ably-cocoa#1380 will have been released. (Internal thread re
this crash and its appearance in these tests is [1].)

[1] https://ably-real-time.slack.com/archives/CSQEKCE81/p1678802535684519
lawrence-forooghian added a commit to ably/ably-asset-tracking-swift that referenced this issue Mar 27, 2023
Resolves #539.

These are based on the corresponding tests in the Android codebase at
commit f6d163f (plus a fix added in eea952b). They stick as closely as
possible to the structure and behaviour of those tests, to which I have
not applied much of a critical eye. The majority of the log messages and
comments are simply copied across.

The only structural difference between these tests and the Android ones
is that I have removed the caller’s responsibility to call
`SubscriberMonitor#close`. There are places in the Android
implementation where we forget to call it, and also it isn’t called if
an error is thrown by `waitForStateTransition`. By making the subscriber
monitor responsible for its own cleanup, we remove these issues.

When I started writing these tests, we were still targeting iOS 12 and
above, which meant it was not possible to use Swift concurrency. Hence,
all of the code is written using either completion handlers or blocking
functions. Since then, we’ve decided to set our deployment target to iOS
13 (see #597), which means we could now use Swift concurrency.  However,
by that point I was well into the writing of these tests and did not
want to re-structure them, especially since they require functionality
that doesn’t exist out of the box in Swift concurrency (for example
timeouts). We may wish to revisit at some point and switch to using
Swift concurrency, but I don’t think it’s urgent.

In order to generate the list of which faults to skip, I ran all of the
tests and checked which ones failed. Whilst doing so, I encountered a
crash in ably-cocoa, which I believe is already reported as
ably/ably-cocoa#1380. In order to get past this issue, I tried using
Marat’s fix from ably-cocoa branch
`fix/1380-dispatch-ARTOSReachability_Callback` (commit 036be28). This
removed the crashes. The list of skipped tests is the list of tests that
failed when using this branch. It then actually turned out that the
non-skipped tests do not exhibit this crash anyway, so I didn’t include
any version change to ably-cocoa in this PR. We can deal with this crash
when we try re-enabling the tests in #575, by which point hopefully the
fix for ably-cocoa#1380 will have been released. (Internal thread re
this crash and its appearance in these tests is [1].)

[1] https://ably-real-time.slack.com/archives/CSQEKCE81/p1678802535684519
lawrence-forooghian added a commit to ably/ably-asset-tracking-swift that referenced this issue Mar 27, 2023
Resolves #539.

These are based on the corresponding tests in the Android codebase at
commit f6d163f (plus a fix added in eea952b). They stick as closely as
possible to the structure and behaviour of those tests, to which I have
not applied much of a critical eye. The majority of the log messages and
comments are simply copied across.

The only structural difference between these tests and the Android ones
is that I have removed the caller’s responsibility to call
`SubscriberMonitor#close`. There are places in the Android
implementation where we forget to call it, and also it isn’t called if
an error is thrown by `waitForStateTransition`. By making the subscriber
monitor responsible for its own cleanup, we remove these issues.

When I started writing these tests, we were still targeting iOS 12 and
above, which meant it was not possible to use Swift concurrency. Hence,
all of the code is written using either completion handlers or blocking
functions. Since then, we’ve decided to set our deployment target to iOS
13 (see #597), which means we could now use Swift concurrency.  However,
by that point I was well into the writing of these tests and did not
want to re-structure them, especially since they require functionality
that doesn’t exist out of the box in Swift concurrency (for example
timeouts). We may wish to revisit at some point and switch to using
Swift concurrency, but I don’t think it’s urgent.

In order to generate the list of which faults to skip, I ran all of the
tests and checked which ones failed. Whilst doing so, I encountered a
crash in ably-cocoa, which I believe is already reported as
ably/ably-cocoa#1380. In order to get past this issue, I tried using
Marat’s fix from ably-cocoa branch
`fix/1380-dispatch-ARTOSReachability_Callback` (commit 036be28). This
removed the crashes. The list of skipped tests is the list of tests that
failed when using this branch. It then actually turned out that the
non-skipped tests do not exhibit this crash anyway, so I didn’t include
any version change to ably-cocoa in this PR. We can deal with this crash
when we try re-enabling the tests in #575, by which point hopefully the
fix for ably-cocoa#1380 will have been released. (Internal thread re
this crash and its appearance in these tests is [1].)

[1] https://ably-real-time.slack.com/archives/CSQEKCE81/p1678802535684519
lawrence-forooghian added a commit to ably/ably-asset-tracking-swift that referenced this issue Mar 27, 2023
Resolves #539.

These are based on the corresponding tests in the Android codebase at
commit f6d163f (plus a fix added in eea952b). They stick as closely as
possible to the structure and behaviour of those tests, to which I have
not applied much of a critical eye. The majority of the log messages and
comments are simply copied across.

The only structural difference between these tests and the Android ones
is that I have removed the caller’s responsibility to call
`SubscriberMonitor#close`. There are places in the Android
implementation where we forget to call it, and also it isn’t called if
an error is thrown by `waitForStateTransition`. By making the subscriber
monitor responsible for its own cleanup, we remove these issues.

When I started writing these tests, we were still targeting iOS 12 and
above, which meant it was not possible to use Swift concurrency. Hence,
all of the code is written using either completion handlers or blocking
functions. Since then, we’ve decided to set our deployment target to iOS
13 (see #597), which means we could now use Swift concurrency.  However,
by that point I was well into the writing of these tests and did not
want to re-structure them, especially since they require functionality
that doesn’t exist out of the box in Swift concurrency (for example
timeouts). We may wish to revisit at some point and switch to using
Swift concurrency, but I don’t think it’s urgent.

In order to generate the list of which faults to skip, I ran all of the
tests and checked which ones failed. Whilst doing so, I encountered a
crash in ably-cocoa, which I believe is already reported as
ably/ably-cocoa#1380. In order to get past this issue, I needed to use
the `main` version of ably-cocoa, which contains a fix for it. I’ve
created #641 to make sure we return to using a proper release of
ably-cocoa before we make our next AAT release.
lawrence-forooghian added a commit to ably/ably-asset-tracking-swift that referenced this issue Mar 27, 2023
Resolves #539.

These are based on the corresponding tests in the Android codebase at
commit f6d163f (plus a fix added in eea952b). They stick as closely as
possible to the structure and behaviour of those tests, to which I have
not applied much of a critical eye. The majority of the log messages and
comments are simply copied across.

The only structural difference between these tests and the Android ones
is that I have removed the caller’s responsibility to call
`SubscriberMonitor#close`. There are places in the Android
implementation where we forget to call it, and also it isn’t called if
an error is thrown by `waitForStateTransition`. By making the subscriber
monitor responsible for its own cleanup, we remove these issues.

When I started writing these tests, we were still targeting iOS 12 and
above, which meant it was not possible to use Swift concurrency. Hence,
all of the code is written using either completion handlers or blocking
functions. Since then, we’ve decided to set our deployment target to iOS
13 (see #597), which means we could now use Swift concurrency.  However,
by that point I was well into the writing of these tests and did not
want to re-structure them, especially since they require functionality
that doesn’t exist out of the box in Swift concurrency (for example
timeouts). We may wish to revisit at some point and switch to using
Swift concurrency, but I don’t think it’s urgent.

In order to generate the list of which faults to skip, I ran all of the
tests and checked which ones failed. Whilst doing so, I encountered a
crash in ably-cocoa, which I believe is already reported as
ably/ably-cocoa#1380. In order to get past this issue, I needed to use
the `main` version of ably-cocoa, which contains a fix for it. I’ve
created #641 to make sure we return to using a proper release of
ably-cocoa before we make our next AAT release. (Note that the huge diff
in the Package.resolved files is the version 2 change that I mentioned
in 034502b.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working. It's clear that this does need to be fixed.
4 participants