delight-im / Android-DDP

[UNMAINTAINED] Meteor's Distributed Data Protocol (DDP) for clients on Android
Apache License 2.0
274 stars 54 forks source link

Meteor.disconnect does not seem to work #68

Closed qbalin closed 8 years ago

qbalin commented 8 years ago

Hi there!

I found the library very easy to read and use, all the doc I need is to read Meteor.java, pretty amazingly clear!

I'm just bumping into a problem: somehow, calling disconnect does not seem to work, i.e. the callback MeteorCallback.onDisconnect is not called after calling disconnect.

I tried something simple, and quite dumb: try to disconnect as soon as the connection is established:

public void onConnect(boolean signedInAutomatically) {
    MeteorSingleton.getInstance().disconnect();
    Log.d(TAG, "METEOR CONNECTED");
}
public void onDisconnect() {
    Log.d(TAG, "METEOR DISCONNECTED");
}

I do get the METEOR CONNECTED message, but it is not followed by the expected METEOR DISCONNECTED. And subscription data keep flowing in.

If I kill my server and try to connect, I will get METEOR DISCONNECTED though, so the callback seems to be called when the connection is down.

Is there something I didn't get? Is the issue known?

Thanks!

ocram commented 8 years ago

Thank you very much for testing this and for posting the report here!

Your setup of disconnecting as soon as being connected actually was not dumb at all :)

The problem you describe indeed seems to be a bug. It might be because we're removing all the callbacks just before closing the connection:

https://github.com/delight-im/Android-DDP/blob/59e79b8e19c9ac74c7e428fa1bb4d50fc9b00b4a/Source/library/src/main/java/im/delight/android/ddp/Meteor.java#L272

What we could do instead is:

qbalin commented 8 years ago

Oh, right! That is probably the problem, thanks for the quick answer!

Also, I amend my previous post: disconnect works well, only the callback is problematic.

Now, I am wondering why disconnect should clear the callbacks at all (and maybe the listeners). Specially since reconnect does not set them back up, and it seems (semantically) that disconnect() + reconnect() = identity().

What do you think about keeping the callbacks (and maybe the listeners) alive after a disconnect? I should try that myself, but I'm not sure how to link the library with my project (I'm using Maven, this library being on it is awesome by the way.)

ocram commented 8 years ago

You're right :)

Again, thank you very much!

This should have been fixed in the master branch now and will be included in the next major release.

In order to test the changes, please temporarily change your Gradle dependency to the following:

compile 'com.github.delight-im:Android-DDP:2f144b3d77'

The list of required (breaking) changes is here.

qbalin commented 8 years ago

Wow! Awesome! I'll test this as soon as I can. Thanks very much, that's quite a reactive fix, I must say!

qbalin commented 8 years ago

Works as advertised, fantastic!

ocram commented 8 years ago

@qbalin Thanks for testing :)