Realtime: dispose method

[ricardopereira]

#111

[tcard]

I recall some comments about this, but some context about why do we want this would be helpful.

Also, I couldn't find this method on other client libs. What's the story there?

[mattheworiordan]

Also, I couldn't find this method on other client libs. What's the story there?

Do you mean dispose? If so, I believe this was introduced because @ricardopereira identified memory issues in our test suite where each invocation of the library was never being freed up, so an explicit dispose to free up the memory was introduced. If you feel this is not valid for an iOS lib, please advise.

[tcard]

Why would iOS be different than any other library? If there is a memory leak, I guess it's because we keep a live reference to the ARTRealtime instance somewhere we shouldn't, right?

[mattheworiordan]

If there is a memory leak, I guess it's because we keep a live reference to the ARTRealtime instance somewhere we shouldn't, right?

I would assume so, although I thought Objective-C had quirky GC support. You will have to make your recommendation as I don't know the specifics of memory management with Objective-C

[tcard]

The quirks of Obj-C/Swift memory management shouldn't affect this case, as far as I can see. Leaks can be caused by exactly the same reasons as in any GCed language (Ruby, Java, etc.), plus cycles, but I don't see a problem of cycles (and if there is, it should be fixed with weak references).

@ricardopereira, could you provide the full story for this method?

[ricardopereira]

Some of the tests were failing with Questionable API usage errors when the Travis server ran all the tests. I digged into it and I found out that some callbacks (e.g.: state event subscriptions) were called after the RealtimeClient were disposed. I fixed some retain cycles that reduced some of the errors but I still had some problems. I checked memory issues with the Instruments tool and I noticed that some RealtimeClients took some time to freed up but in the end all the instances were getting cleanup. So, I decided to explicitly clean the channels and events on every test invocation just for a precaution.

Another example where I decided to clean the channels and events just for convenience:

context("test") {
    let options = AblyTests.commonAppSetup()
    options.autoConnect = false
    let client = ARTRealtime(options: options)

    it("one test") {
        client.connect()
        defer {
            // Without dispose
            //client.dispose()
            client.close()
        }

        waitUntil(timeout: testTimeout) { done in
            client.eventEmitter.on { state, error in
                print("1. received state \(state)")
                done()
            }
        }
    }

    it("second test") {
        client.connect()
        defer {
            // Without dispose
            //client.dispose()
            client.close()
        }

        waitUntil(timeout: testTimeout) { done in
            client.eventEmitter.on { state, error in
                print("2. received state \(state)")
                done()
            }
        }
    }
}

Result:

Connection__test__one_test_UsersricardopereiraRepositoriesWhitesmithablyiosablySpecRealtimeClientconnectionswift_401]' started.
1. received state ARTRealtimeConnectionState
1. received state ARTRealtimeConnectionState
1. received state ARTRealtimeConnectionState
Test Case '-[ablySpec.RealtimeClientConnection Connection__test__one_test_UsersricardopereiraRepositoriesWhitesmithablyiosablySpecRealtimeClientconnectionswift_401]' passed (0.019 seconds).
Test Case '-[ablySpec.RealtimeClientConnection Connection__test__second_test_UsersricardopereiraRepositoriesWhitesmithablyiosablySpecRealtimeClientconnectionswift_415]' started.
1. received state ARTRealtimeConnectionState
2. received state ARTRealtimeConnectionState
1. received state ARTRealtimeConnectionState
2. received state ARTRealtimeConnectionState
1. received state ARTRealtimeConnectionState
2. received state ARTRealtimeConnectionState
Test Case '-[ablySpec.RealtimeClientConnection Connection__test__second_test_UsersricardopereiraRepositoriesWhitesmithablyiosablySpecRealtimeClientconnectionswift_415]' passed (0.014 seconds).

[tcard]

If this method is used as a test convenience, I'd say it should be kept in the tests, don't you think? Either as an extension or as just a function.

There are some other methods (like removeAllChannels) that are not part of the documented API and mainly bloat the API, in my opinion. I think it's really important to keep the interface clean and minimal.

[mattheworiordan]

There are some other methods (like removeAllChannels) that are not part of the documented API and mainly bloat the API, in my opinion

Agreed. In the case of Ruby there is a convention to document some methods as private APIs, even though they are not private methods, but even in those cases I have kept this to a minimum as the less exposed methods that are not part of the API the better.

[ricardopereira]

Well, I never have in mind to keep the API interface at the same level from other platforms but it makes sense. I added removeAllChannels because I thought it would be helpful but it is not my call to decide those changes. Lesson learned.

[ricardopereira]

So, I will include dispose only for the test suite.

@mattheworiordan Just to clarify one thing: if the user closes the connection, should the library clean the pendingMessages? AFAIK, queuedMessages should remain for the next connection, right?

[mattheworiordan]

Just to clarify one thing: if the user closes the connection, should the library clean the pendingMessages? AFAIK, queuedMessages should remain for the next connection, right?

Well if you look at http://docs.ably.io/client-lib-development-guide/features/#RTN18b and http://docs.ably.io/client-lib-development-guide/features/#RTN18c it states that in suspended or failed states the messages should be marked as failed. As the CLOSED state is not transient, the messages should also be treated as failed as the client library is no longer trying to send them.