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

Connection resume does not fire channel detached #538

Closed
mattheworiordan opened this issue Nov 22, 2016 · 7 comments
Closed

Connection resume does not fire channel detached #538

mattheworiordan opened this issue Nov 22, 2016 · 7 comments
Assignees
Labels
bug Something isn't working. It's clear that this does need to be fixed.

Comments

@mattheworiordan
Copy link
Member

@ricardopereira please can we write a test to confirm what happens to a channel when a connection resume fails. What should happen is that all channels that were attached become detached, the state change is emitted along with an error. Please see http://docs.ably.io/client-lib-development-guide/versions/v0.8/features/#RTN15c3 for 0.8 spec.

A customer has reported the detached event not firing.

@mattheworiordan mattheworiordan added the bug Something isn't working. It's clear that this does need to be fixed. label Nov 22, 2016
@mattheworiordan
Copy link
Member Author

@ricardopereira whilst reviewing the verbose logs, I can unfortunately see that the client library does not output the URL it is connecting to for new WS connections. Can you please ensure all HTTP and WebSocket connections / requests output the complete URL with headers if appropriate when verbose logging is on?

@mattheworiordan
Copy link
Member Author

@ricardopereira unfortunately I also now suspect (but do not have proof as the logs are concatenated meaning I cannot prove this) that the resume following an auth token change is not working. I suspect the resume arguments for the WS connection are not included.

Can you additionally write a test that uses a low value TTL for the token, attaches to a channel, then waits for the auth to complete renewing the token, and then publish over REST to that channel to make sure it's received?

@mattheworiordan
Copy link
Member Author

FYI, in the connected message following auth, I see:

{
    action = 4;
    connectionDetails =     {
        clientId = "REDACTED";
        connectionKey = "REDACTED";
        connectionStateTtl = 120000;
        maxFrameSize = 262144;
        maxIdleInterval = 15000;
        maxInboundRate = 50;
        maxMessageSize = 65536;
        serverId = "frontend.d8d9.1.eu-west-1-A.i-5a33dacc";
    };
    connectionId = REDACTED;
    connectionKey = "REDACTED";
    connectionSerial = "-1";
}

If the resume failed, we should have received an error with the connected message with an 80008 error code. As you can see above, that did not happen.

So we need to check that:

  • Even with the absence of an error, if the connection ID changes, then all channels should become detached (the error is only informational and not the reason channels are detached)
  • Auth renewal triggers a reconnect with resume and resumes connections
  • Auth renewal triggers a reconnect with (fabricated) faulty resume, resumes connection, emits an error, and detaches all channels.

@ricardopereira
Copy link
Contributor

@mattheworiordan Sorry for the delay. There's already a test for RTN15c3 but it's pending.

@ricardopereira
Copy link
Contributor

@mattheworiordan I run that test and it passes. I will implement the additional test.

@ricardopereira
Copy link
Contributor

ricardopereira commented Nov 23, 2016

Please test using pod 'Ably', :git => 'https://github.com/ably/ably-ios.git', :branch => 'fix-538'. Hopefully it works with those fixes.

UPDATE: the branch fix-528 has been merged. Please test it using pod 'Ably', :git => 'https://github.com/ably/ably-ios.git', :branch => 'master'

@mattheworiordan
Copy link
Member Author

This has been tested by the customer and works. Thanks @ricardopereira

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.
Development

No branches or pull requests

2 participants