Fix RTN20. #444

tcard · 2016-05-02T12:40:57Z

I've tested the ARTOSReachability "live" manually (by switching on/off
the real Internet connection on my machine) and it works as expected,
but it needs #385 to properly handle transport errors.

mattheworiordan · 2016-05-03T16:39:37Z

Spec/RealtimeClientConnection.swift

                            }
+                            // let stateChange = stateChange!


Is this meant to be commented out?

No, sorry, leftover. Fixed at 65bfc55.

mattheworiordan · 2016-05-03T16:42:36Z

LGTM

ricardopereira · 2016-08-23T21:37:51Z

@mattheworiordan I rebased it with master.
RTN20 tests are passing and they look good.
Can I force push? /cc @tcard

mattheworiordan · 2016-08-24T08:17:49Z

Yes, please force push. Are any other tests failing?

ricardopereira · 2016-08-24T08:26:57Z

@mattheworiordan Yes, there is at least one test failing with something related with this PR:

Test Case '-[AblySpec.Auth Token__token_auth_and_clientId__Incompatible_client__with_Realtime__it_should_change_the_connection_state_to_FAILED_and_emit_an_error]' started.
INFO: R:0x7fc6daa1ce80 ARTRealtime closed
INFO: Reachability: stopped listening for host (null)
ERROR: R:0x7fc6daa377c0 WS:0x7fc6daa38f80 ARTWebSocketTransport: token auth failed with ARTErrorInfo with code 401, message: incompatible credentials
EXC_BAD_ACCESS

mattheworiordan · 2016-08-24T09:06:22Z

@ricardopereira well from the title of that test, it should raise an incompatible credentials error I believe, so it sounds like that is a test that genuinely needs to be fixed then.

ricardopereira · 2016-08-24T09:47:51Z

@mattheworiordan The test is fine. I made a fix and it is running now.
I'm running all the test suite on my machine.

tcard · 2016-08-24T10:52:42Z

Source/ARTOSReachability.m

+- (void)listenForHost:(NSString *)host callback:(void (^)(BOOL))callback {
+    [self off];
+
+    if (!host) {


Why would host be nil here? If it's because the user provides a nil host, this validation should happen at a higher level, where the user provides it, and properly return an error to the user (probably as an exception). Internal code can just assume then that host is correct.

@tcard It makes sense what you said but I think this check is important to help out when you're debugging. The host was nil because the transport was nil. Now, why was the transport nil? I don't know. I need to check the code.

Just for the sake of clarification: the transport is nil because the client transitions to FAILED, see https://github.com/ably/ably-ios/blob/4f1bd1098c54663ce20cf3dfb7b4595f4fbe4b40/Source/ARTRealtime.m#L302.

@tcard I'm sure you faced something similar when you run the test suite on your machine.

The force cast of a transport like realtime.transport as! TestProxyTransport sometimes raises a bad access because the realtime.transport is created only when the connection starts CONNECTING, so we need to guarantee that the current connection has a valid transport instance before force casting it. The test suite will stall when a EXC_BAD_ACCESS occurs.

Then we have as well situations like this:

Just for the sake of clarification: the transport is nil because the client transitions to FAILED, see https://github.com/ably/ably-ios/blob/4f1bd1098c54663ce20cf3dfb7b4595f4fbe4b40/Source/ARTRealtime.m#L302.

What is your opinion on creating the transport once and release it only when the RealtimeClient deinitialise?

Well, I don't know how much that would affect things, but I suspect we would be fixing the wrong problem. If we're trying to use the transport when there is no transport, forcing a transport to be there even when there's no connection will probably just move the failure elsewhere.

Instead, things requiring a transport should make sure that there's indeed a transport present I think.

Why does the client transition to FAILED?

AFAICS that doesn't fix the underlying issue, just improves error reporting (which is good, but that's the kind of changes I was talking about here arguing that they're not critical right now.)

If we have a test in which the transport is unexpectedly nil, why not fix it so that the transport is not nil?

@tcard Yes, you're right. The error reporting is another discussion.

The transport is unexpectedly nil because when the client connects, the _transport is nil but the instance is created and starts the connection, then the _transport emits the Failed state because of the incompatible client, the ARTRealtimeFailed is handled and the _transport is cleared, then comes the _reachability listenForHost:[_transport host] and _transport is nil.

@tcard Now, when is the right moment to start listening for a host?

IMO, if the connections fails immediately then it doesn't makes sense to start listening for a host. So, if there is no transport at the moment when the client is setting up the OS reachability, then ignore it.

then comes the _reachability listenForHost:[_transport host] and _transport is nil.

I guess you mean that [_transport host] is nil? _transport cannot be nil at this point since it's instantiated right above.

Now, why is [_transport host] nil? That surely looks like a inconsistent state of _transport.

@tcard No, I mean _transport is nil. The connection will emit a Failed state and set _transport to nil.

ricardopereira · 2016-08-25T10:51:38Z

Legacy tests:

ricardopereira · 2016-08-25T10:52:51Z

Specs:

Test Suite 'AblySpec.xctest' failed at 2016-08-25 10:57:51.589.
     Executed 520 tests, with 6 failures (0 unexpected) in 417.708 (418.077) seconds
Test Suite 'All tests' failed at 2016-08-25 10:57:51.591.
     Executed 520 tests, with 6 failures (0 unexpected) in 417.708 (418.080) seconds

Failed:

/Users/ricardopereira/Repositories/Whitesmith/ably-ios/Spec/RealtimeClientPresence.swift:907: error: -[AblySpec.RealtimeClientPresence Presence__enter__should_result_in_an_error_if_the_client_does_not_have_required_presence_permission] : expected to contain <Channel denied access based on given capability>, got <Invalid clientId for credentials>
Test Case '-[AblySpec.RealtimeClientPresence Presence__enter__should_result_in_an_error_if_the_client_does_not_have_required_presence_permission]' failed (0.894 seconds).

/Users/ricardopereira/Repositories/Whitesmith/ably-ios/Spec/RealtimeClientPresence.swift:980: error: -[AblySpec.RealtimeClientPresence Presence__update__should_result_in_an_error_if_the_client_does_not_have_required_presence_permission] : expected to contain <Channel denied access based on given capability>, got <Invalid clientId for credentials>
Test Case '-[AblySpec.RealtimeClientPresence Presence__update__should_result_in_an_error_if_the_client_does_not_have_required_presence_permission]' failed (0.859 seconds).

/Users/ricardopereira/Repositories/Whitesmith/ably-ios/Spec/RealtimeClientPresence.swift:1221: error: -[AblySpec.RealtimeClientPresence Presence__leave__should_result_in_an_error_if_the_client_does_not_have_required_presence_permission] : expected to contain <Channel denied access based on given capability>, got <Invalid clientId for credentials>
Test Case '-[AblySpec.RealtimeClientPresence Presence__leave__should_result_in_an_error_if_the_client_does_not_have_required_presence_permission]' failed (0.918 seconds).

/Users/ricardopereira/Repositories/Whitesmith/ably-ios/Spec/RealtimeClientPresence.swift:1885: error: -[AblySpec.RealtimeClientPresence Presence__history__should_return_a_PaginatedResult_page] : expected to all pass a condition, but failed first at element <<ARTPresenceMessage: 0x7fb114258ea0> {id: 
...
/Users/ricardopereira/Repositories/Whitesmith/ably-ios/Spec/RealtimeClientPresence.swift:1900: error: -[AblySpec.RealtimeClientPresence Presence__history__should_return_a_PaginatedResult_page] : expected to all pass a condition, but failed first at element <<ARTPresenceMessage: 0x7fb11417f610> {id: 
...
Test Case '-[AblySpec.RealtimeClientPresence Presence__history__should_return_a_PaginatedResult_page]' failed (2.681 seconds).

/Users/ricardopereira/Repositories/Whitesmith/ably-ios/Spec/RestClientPresence.swift:501: error: -[AblySpec.RestClientPresence Presence__presence_messages_retrieved_are_decoded_in_the_same_way_that_messages_are_decoded] : expected to equal <{test = 1;}>, got <nil> (use beNil() to match nils)
2016-08-25 10:56:47.359 xctest[77206:4299336] INFO: Reachability: stopped listening for host sandbox-realtime.ably.io
Test Case '-[AblySpec.RestClientPresence Presence__presence_messages_retrieved_are_decoded_in_the_same_way_that_messages_are_decoded]' failed (1.507 seconds).

Same failures comparing with #467.

ricardopereira · 2016-08-25T11:00:20Z

@tcard @mattheworiordan Ready to be reviewed.

tcard · 2016-08-25T12:48:04Z

LGTM

mattheworiordan reviewed May 3, 2016
View reviewed changes

tcard force-pushed the reachability branch 2 times, most recently from 3fa63a8 to 864650e Compare May 4, 2016 16:55

tcard mentioned this pull request May 5, 2016

RTN17a #387

Merged

tcard force-pushed the reachability branch 3 times, most recently from d1bc9b9 to e71b852 Compare May 12, 2016 00:34

ricardopereira force-pushed the reachability branch from aeae1e2 to c78004f Compare August 24, 2016 08:29

tcard reviewed Aug 24, 2016
View reviewed changes

ricardopereira mentioned this pull request Aug 25, 2016

CI: update CocoaPod to v1.0.1 #467

Merged

tcard closed this Aug 26, 2016

tcard force-pushed the reachability branch from b1c0eac to c88fd41 Compare August 26, 2016 17:31

tcard mentioned this pull request Aug 26, 2016

'Socket not connected' incorrectly leads to FAILED connection state rather than DISCONNECTED #471

Closed

ricardopereira deleted the reachability branch October 12, 2016 06:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix RTN20. #444

Fix RTN20. #444

tcard commented May 2, 2016

mattheworiordan May 3, 2016

tcard May 3, 2016

mattheworiordan commented May 3, 2016

ricardopereira commented Aug 23, 2016

mattheworiordan commented Aug 24, 2016

ricardopereira commented Aug 24, 2016 •

edited

Loading

mattheworiordan commented Aug 24, 2016

ricardopereira commented Aug 24, 2016

tcard Aug 24, 2016

ricardopereira Aug 24, 2016

ricardopereira Aug 24, 2016

ricardopereira Aug 24, 2016

tcard Aug 24, 2016

tcard Aug 24, 2016

ricardopereira Aug 24, 2016

ricardopereira Aug 24, 2016

tcard Aug 24, 2016

ricardopereira Aug 24, 2016

ricardopereira commented Aug 25, 2016

ricardopereira commented Aug 25, 2016 •

edited

Loading

ricardopereira commented Aug 25, 2016

tcard commented Aug 25, 2016

Fix RTN20. #444

Fix RTN20. #444

Conversation

tcard commented May 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattheworiordan commented May 3, 2016

ricardopereira commented Aug 23, 2016

mattheworiordan commented Aug 24, 2016

ricardopereira commented Aug 24, 2016 • edited Loading

mattheworiordan commented Aug 24, 2016

ricardopereira commented Aug 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricardopereira commented Aug 25, 2016

ricardopereira commented Aug 25, 2016 • edited Loading

ricardopereira commented Aug 25, 2016

tcard commented Aug 25, 2016

ricardopereira commented Aug 24, 2016 •

edited

Loading

ricardopereira commented Aug 25, 2016 •

edited

Loading