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

Manage susbscriptions after disconnect #144

Open vfrico opened 7 years ago

vfrico commented 7 years ago

I couldn't find a proper documentation about disconnect method. On my application usually I disconnect the meteor connection to avoid sending messages and save battery, but when I reconnect it I realized that I don't receive updates of a subscription unless I subscribe again. I have been looking for documentation about disconnect method (on Readme in this repo and in the Java code), but I haven't find what is the exact behavior when disconnecting (with mMeteor.disconnect()) and reconnecting again (with mMeteor.reconnect()).

I would like to know if there is any way to manage this situation other than tracking which are the subscriptions you use and manage them with an auxiliary Class. Or at least a method to get all active subscriptions.

Thanks

ocram commented 7 years ago

Excellent point, thank you!

The cause of this behavior may be that the session ID, which is declared here and set here as received from the server after the connection has been established, is cleared when disconnecting from the server.

If we then re-connect with a new session ID, the server has no way to know what subscriptions to send to us. They’re lost. So this commit is an attempt to fix this issue by not dropping the session ID.

You can test the version with the potential fix by changing your Gradle dependency to the following:

compile 'com.github.delight-im:Android-DDP:0f743e22b7c4405c95695ca58f50e2ad9b1ff5e1'

Does that change anything?

vfrico commented 7 years ago

Nope, the problem has not been solved using that version of the library. I have been inspecting the mSessionID variable across an execution of the application, and tried several executions. If I don't do any disconnect, the session ID will be always the same, but when calling to disconnect, when the server reconnects, sends a new session ID (or, at least, this is the behavior I can extract from this (cropped) log):

Meteor_id: 98J2ofghY6xyNuSNe

 Meteor
   onOpen
 Meteor
   send
     message == {"msg":"connect","support":["1","pre2","pre1"],"session":"98J2ofghY6xyNuSNe","version":"1"}
     dispatching
 Meteor
   onTextMessage
     payload == {"server_id":"0"}
 Meteor
   onTextMessage
     payload == {"msg":"connected","session":"2PGdyj5WkSCnG4ogR"}
 Meteor
   send
     message == {"msg":"method","id":"03c60e15-2143-4c96-9011-c3a3b622253e","params":[{"resume":"Mepu9vxA2o8qaZ5EvElHJHhLOYCTx7DnTZj6kIgGmUd"}],"method":"login"}
     dispatching
 Meteor
   onTextMessage
     payload == {"msg":"added","collection":"users","id":"vn397TWBuP55o68r5",
                "fields":{"emails":[{"address":"*vfrico*","verified":true}],
                "profile":{"type":"human","layers":[{"privilege":"public","id":"ROUTESMAD","sendTo":"False"}],
                "description":""},"username":"*vfrico*"}}

Meteor_id: 2PGdyj5WkSCnG4ogR

My application has a wrapper for this Meteor DDP library, so we have simulated the desired behavior by tracking with lists the topics subscribed. We also have created two simple methods, pause() and resume() to manage a pause for the server. I don't know if this solution is too ad-hoc and would worth to integrate on the library and publish a PR with it...

Thanks

ocram commented 7 years ago

Thanks a lot for testing and debugging this!

the session ID will be always the same, but when calling to disconnect, when the server reconnects, sends a new session ID (or, at least, this is the behavior I can extract from this (cropped) log)

That’s also what I read from your log. The client-to-server message

{"msg":"connect",...,"session":"98J2ofghY6xyNuSNe",...}

really should ask the server to pick up this session ID and re-use it. The server then responding with

{"msg":"connected","session":"2PGdyj5WkSCnG4ogR"}

is a bit strange but could still mean that the server picked up the ID correctly and just refreshed the ID, keeping any associated state. But that’s apparently not what happened. From your log, however, it is not obvious if this really did not happen. Are you really sure that subscriptions are not remembered anymore from that point on?

From what I can read in the specification of the protocol, which is a bit vague and lacks detail, what we do is correct here. So I would say this may be a Meteor issue or at least something where we don’t know if this is the behavior they expect. Perhaps keeping subscriptions across connections (albeit with the same session ID) is not something they have actually implemented.

Your wrapper and the workaround sound interesting, and could really be something for this library. But at this point I feel we would rather be hiding the symptoms and concealing what’s actually happening, ignoring the underlying bugs. So if there’s any chance we could fix something here or the Meteor project could clear things up, I’d rather do that.

vfrico commented 7 years ago

I do not have a total control over the remote Meteor server. I will ask for more information about if this is a server issue or a client issue. I have checked and with my workaround I receive the subscriptions after a pause->resume cycle, but with the actual branch issue-144 I don't get any updates after a disconnect->reconnect cycle.

As you say, it is better to solve the actual problem rather than having a workaround.

ocram commented 7 years ago

Thanks!

I do not have a total control over the remote Meteor server. I will ask for more information about if this is a server issue or a client issue.

No problem. It really seems like a server issue. Or we got something wrong with our understanding of the protocol. It doesn’t look like a simple implementation issue in the client.

I have checked and with my workaround I receive the subscriptions after a pause->resume cycle, but with the actual branch issue-144 I don't get any updates after a disconnect->reconnect cycle.

Too bad! We can confirm that the provided (old) session ID is not picked up by the server (which just sends a new one). Additionally, it seems the server does not remember the previous subscriptions, either.

I see three possibilities here for why this happens:

  1. Meteor’s protocol (DDP) does not support this and it was never meant to be supported.
  2. There’s a bug in Meteor’s protocol (DDP) with regard to this issue (or the two symptoms we’ve seen).
  3. We’ve misunderstood how Meteor’s protocol (DDP) is supposed to work.
vfrico commented 7 years ago

Finally, I couldn't get any information about the remote server, so I can't go too far on this issue. I don't have time to fire up a Meteor server and research where the error could be.

Without having clear how the Meteor server works, maybe when executing disconnect() is possible that the server forget that sessionID and that may be the reason why sends a new sessionID. Maybe this feature is used to reconnect after a network problem rather than after the client has executed a disconnect().

vfrico commented 7 years ago

Well, I have found this searching a bit on Google: Meteor Server's Connection

Currently when a client reconnects to the server (such as after temporarily losing its Internet connection), it will get a new connection each time. The onConnection callbacks will be called again, and the new connection will have a new connection id.

In the future, when client reconnection is fully implemented, reconnecting from the client will reconnect to the same connection on the server: the onConnection callback won’t be called for that connection again, and the connection will still have the same connection id.

So, this means that is a possible future upgrade :weary:

Edit: As a reference, I have implemented on the library something similar than I have on my wrapper. It is not tested: https://github.com/vfrico/Android-DDP/commit/3e68cb9b1809f36cbe484202dc285664c61dcf71

ocram commented 7 years ago

Without having clear how the Meteor server works, maybe when executing disconnect() is possible that the server forget that sessionID and that may be the reason why sends a new sessionID. Maybe this feature is used to reconnect after a network problem rather than after the client has executed a disconnect().

That would clearly have been the most obvious explanation. We also thought that the problem occurs only with manual disconnects which may intentionally destroy the old session. But that’s not actually the case. If the disconnect happens due to network problems, the old session is not picked up, either.

Well, I have found this searching a bit on Google: Meteor Server's Connection

It is not entirely clear whether this is really talking about the exact same thing, because it uses different vocabulary. But unfortunately, it really seems that this has not been implemented in Meteor yet, although the DDP specification supports it.

As a reference, I have implemented something similar: https://github.com/vfrico/Android-DDP/commit/3e68cb9b1809f36cbe484202dc285664c61dcf71

Looks good. I would definitely prefer the behavior to be fixed in a clean way, possibly requiring a fix in Meteor, but we may not get that. Otherwise, implementing that workaround in the basic methods for reconnecting (or just connecting in general) would probably be preferable to adding another set of methods with new semantics. And finally, with that solution, the server will probably re-send all the data after reconnecting. That is not optimal, but maybe the best we can get.

vfrico commented 7 years ago

Looks good. I would definitely prefer the behavior to be fixed in a clean way, possibly requiring a fix in Meteor, but we may not get that. Otherwise, implementing that workaround in the basic methods for reconnecting (or just connecting in general) would probably be preferable to adding another set of methods with new semantics. And finally, with that solution, the server will probably re-send all the data after reconnecting. That is not optimal, but maybe the best we can get.

I didn't add a PR because I know is not the preferrable way to go, and might not be included on the library, but can be useful to others coming from google to solve this issue.

This seems like a dead end, so I'm closing this issue

ocram commented 7 years ago

but can be useful to others coming from google to solve this issue

Definitely! And who knows, perhaps we implement your solution as well. I’m just not sure about this, yet.

This seems like a dead end, so I'm closing this issue

Sorry for that annoying situation here, but let’s keep the issue open. We have to keep in mind that this is something that we have to fix, wait for an upstream fix, or implement a workaround.