1074665 - PushHandler registration logic should be isolated from rooms implementation details

Status: RESOLVED INCOMPLETE

(Hello (Loop) :: Client, enhancement, P5)

Comment 1

[Matthew N. [:MattN]]

> * Provide a mechanism so that updates can be pushed to the panel/conversation views via MozLoopAPI      

We can use notifyStatusChanged for this or do a similar thing where an observer notification is converted into a DOM Event.

Comment 2

[Paul Kerr [:pkerr]]

Attachment: Update rooms store on notification

WIP: need to complete unit tests

Comment 3

[Paul Kerr [:pkerr]]

Is there a notification when a room is deleted? What would such an event look like in the room list returned when I perform a GET rooms/?version=xxxx after receiving a notification? Just seeing a room missing from a version-ed request would only imply that nothing had changed.

Comment 4

[Adam Roach [:abr]]

(In reply to Paul Kerr [:pkerr] from comment #3)
> Is there a notification when a room is deleted? What would such an event
> look like in the room list returned when I perform a GET rooms/?version=xxxx
> after receiving a notification? Just seeing a room missing from a version-ed
> request would only imply that nothing had changed.

Ooh, good question. You'll definitely see a push notification, but the means of conveying that the room is gone aren't currently defined.

What I would propose is adding an new field, "deleted". If present and true, it indicates that the room associated with the indicated token is gone; basically, something like:

HTTP/1.1 200 OK
Connection: keep-alive
Content-Length: 30
Content-Type: application/json; charset=utf-8
Date: Wed, 16 Jul 2014 13:23:04 GMT
ETag: W/"1e-2896316483"
Timestamp: 1405516984

[
    {
        "roomToken": "_nxD4V4FflQ",
        "deleted: true
    }
]

Alexis / Paul -- does that sound like a reasonable approach to you?

Comment 5

[Paul Kerr [:pkerr]]

That is clear indication on the notification side. It works for me.

Comment 6

[Alexis Metaireau (:alexis)]

I would rather use the 410 GONE status rather than having this "deleted" attribute. Also, I believe that after some time the rooms will be just deleted from the server and as such we would return a 404 as well.

Would this approach work for you?

Comment 7

[Adam Roach [:abr]]

(In reply to Alexis Metaireau (:alexis) from comment #6)
> I would rather use the 410 GONE status rather than having this "deleted"
> attribute. Also, I believe that after some time the rooms will be just
> deleted from the server and as such we would return a 404 as well.
> 
> Would this approach work for you?

Ah, sorry -- I should have been clearer in my request.

For |GET /rooms/3jKS_Els9IU HTTP/1.1|, what you propose makes perfect sense. I would expect something like a 410 or 404 in that case.

We're talking about |GET /rooms?version=<version> HTTP/1.1|, where we might find out about changes to several resources at a time. For example, the response might need to say "room X has been deleted, and room Y has been added." With my proposal, this would look like:

[
    {
        "roomToken": "_nxD4V4FflQ",
        "deleted": true
    },
    {
        "roomToken": "QzBbvGmIZWU",
        "roomName": "Room Y",
        "roomUrl": "http://localhost:3000/rooms/QzBbvGmIZWU",
        "maxSize": 2,
        "currSize": 0,
        "ctime": 1405517418
    }
]

With that clarification, does what I'm proposing make more sense?

Comment 8

[Paul Kerr [:pkerr]]

So, the sequence would be:
1) notification is sent via the PushServer, incrementing the version xxxx.
2) loop client requests a list of updated rooms via GET /rooms?version=xxxx.
3) loop client makes a GET /rooms/<token> call for each of the rooms found in the list returned from the previous call.
4) a GET /rooms/<token> request will receive a 410 GONE status if the room with the token has been deleted.

Since that is the flow the client goes through currently to determine what has been changed the 410 can be handled if it works as I have detailed above.

Comment 9

[Adam Roach [:abr]]

(In reply to Paul Kerr [:pkerr] from comment #8)
> So, the sequence would be:
> 1) notification is sent via the PushServer, incrementing the version xxxx.
> 2) loop client requests a list of updated rooms via GET /rooms?version=xxxx.
> 3) loop client makes a GET /rooms/<token> call for each of the rooms found
> in the list returned from the previous call.
> 4) a GET /rooms/<token> request will receive a 410 GONE status if the room
> with the token has been deleted.
> 
> Since that is the flow the client goes through currently to determine what
> has been changed the 410 can be handled if it works as I have detailed above.

There are a couple of problems here.

The first is that the information from the single |GET /rooms?version=xxxx| call should return enough information for you to handle processing of any room that you're not in. If you find a need to iterate through the rooms and do a |GET /rooms/<token>| on each of them, then I've botched the API and we need to fix |GET /rooms?version=xxxx|. I really don't want a client with five rooms to do six GETs just because they got a single push notification -- that's a recipe for disaster.

The second is that you're proposing that |GET /rooms?version=xxxx| should show the presence of a room that doesn't actually exist any longer, in a way that's indistinguishable from an existing room. That's just going to cause confusion.

When it comes down to it, the "?version=xxxx" variation is a shortcut for getting the whole state, akin to asking for a patch on a codebase rather than getting the whole codebase. Right now, the defined protocol allows communicating *additions* and *changes*. What it's missing is a format for *deletions*. We need to add one, and the proposal I'm making is the simplest way of doing so that I can easily devise.

Tagging pkerr for needinfo, mostly to verify that we don't currently need to iterate through all of a user's rooms on a push notification, and to confirm that anything that leads us down a path of doing should be flagged to me so we can discuss alternate approaches.

Comment 10

[Paul Kerr [:pkerr]]

The client does not run through the entire list of rooms when a notification is received. It runs through only those rooms that are returned from the /rooms?version request. That is, those rooms that have changed in that version.

I feel that getting the room details is necessary since all that I can tell from the room list is if the room name or currSize has changed. But, even if currSize is the same the participants could have changed with one leaving and another entering. I can tell which room is active by looking at the currSize but the code at this point has no idea what the UI is displaying. It sends the update details on to registered entities in the content side.

Comment 11

[Adam Roach [:abr]]

(In reply to Paul Kerr [:pkerr] from comment #10)
> The client does not run through the entire list of rooms when a notification
> is received. It runs through only those rooms that are returned from the
> /rooms?version request. That is, those rooms that have changed in that
> version.
> 
> I feel that getting the room details is necessary since all that I can tell
> from the room list is if the room name or currSize has changed. But, even if
> currSize is the same the participants could have changed with one leaving
> and another entering. I can tell which room is active by looking at the
> currSize but the code at this point has no idea what the UI is displaying.
> It sends the update details on to registered entities in the content side.

Okay, so it's looking increasingly like the aggregated state needs to simply be an array of the exact same state that would be returned by getting each room individually. I worry about the server performance implications of kicking off multiple HTTP transactions in response to a simple push, which is why we moved to have different push endpoints for different kinds of state.

Alexis: are you okay with unifying the formats here, where the body of |GET /rooms/version=xxxx| is simply an array containing objects that look just like those you get from |GET /rooms/{token}|? We'd add "token" to the objects, but they'd otherwise be as defined here: https://wiki.mozilla.org/Loop/Architecture/Rooms#GET_.2Frooms.2F.7Btoken.7D

Comment 12

[Alexis Metaireau (:alexis)]

We currently aren't storing any history for the rooms, so returning their state is probably a good thing to do.

I would need to know for how long do we need to store the history of a room after it's been deleted on the server side ?

Apart from that, it seems completely okay to return the list of rooms and their state *since the version was issued*.

Comment 13

[Paul Kerr [:pkerr]]

Attachment: Update rooms store on notification

WIP

Comment 14

[Adam Roach [:abr]]

I've updated the API description to match the outcome of this discussion. Please double-check my changes to make sure they match your understanding: https://wiki.mozilla.org/index.php?title=Loop%2FArchitecture%2FRooms&diff=1027249&oldid=1023877

Comment 15

[Paul Kerr [:pkerr]]

Yes, this change looks good to me. A one pass operation will help the client implementation.

Comment 16

[Alexis Metaireau (:alexis)]

Yes that reflects my understanding, perfect!

For implementation, I need to know for how long we need to hold this information on the server.

Thanks!

Comment 17

[Adam Roach [:abr]]

(In reply to Alexis Metaireau (:alexis) from comment #16)
> For implementation, I need to know for how long we need to hold this
> information on the server.

tl;dr: If the server doesn't need to keep this state for any other reason, use 27 minutes. If the client asks for a version older than 27 minutes ago, return an error with a new, distinct code. If the client gets this new code, it needs to pull a full list of rooms instead of using "?version=<version>".

----

In previous conversations, you'd mentioned the use of "410 Gone" when people try to use a deleted room, which is presumably a different code than when people try to use a room that has never existed (which I assume will return a 404). If this is your intention, then  you'll need a tombstone for the room for a pretty long time (on the order of days or weeks), which would be more than sufficient for the purpose of this bug.

If you're not holding the state to distinguish between "deleted rooms" and "tokens that were never a room" for that (or some other) reason, then we're down to what this bug requires. In normal operation, the requirement is that the state is around for long enough for the client to get the push notification and then perform the GET. These operations are all over TCP, which means that technically each of these messages can be delayed by network loss for up to 9 minutes. There are three TCP operations in involved here (Loop Server to Simple Push Server; Simple Push Server to client; and client to Loop Server), so the theoretical maximum is 27 minutes.

That said, these things always find a way to operate outside the theoretical maximum, and we should be able to handle such situations. Since we're discarding information necessary to know about a proper delta after some period of time, then we need to indicate that we *can't* communicate the changes as a delta, and instead ask the client to retrieve a full set of data; basically, an error code that means "I can't tell you what has changed since that version, so you need to just ask for the whole state again."

Comment 18

[Alexis Metaireau (:alexis)]

Thanks for the answer,

If that's possible, I would like to drop the fact that we know a room had been deleted after some amount of time, and that's what I'm asking for here. In other words, after some time the server will always answer 404, even if the room existed a long time ago, because that doesn't make sense to keep this information for ever.

What I'm trying to find out is what should be the maximum time we keep this information. During this period, we will be able to answer the GET on rooms with the "deleted = true" attribute, and after this time, the information will not be present (and if accessed, a room will just say it doesn't exist).

Would that be acceptable, and if yes, what would be the time we need to store this information? 3*9 seems like the time needed for the client to be alerted and get the information it needs, so we could use this TTL value, I believe.

Comment 19

[Paul Kerr [:pkerr]]

Attachment: Update rooms store on notification

ebased to apply after fx-team (assumed to already incorporate 1074663) after 1074664 and 1074699: fx-team -> 664 -> 699 -> 665

Comment 20

[Paul Kerr [:pkerr]]

WIP: still needs to handle a room delete as part of a server notification. Need to track the decision of the server team as to the final disposition of handling a delete.

Comment 21

[Paul Kerr [:pkerr]]

Attachment: Update rooms store on notification

Comment 22

[Mike de Boer [:mikedeboer]]

Paul, is it OK if I steal this one?

Comment 23

[Mike de Boer [:mikedeboer]]

Attachment: WIP: new push handler

Comment 24

[Mike de Boer [:mikedeboer]]

Attachment: Part 1: refactor PushHandler to encapsulate more of the registration flow, simplify the API and more

Matt, do you have some time today to take a look at this to see if you like this approach and have some/ any suggestions to improve things, possibly?
I know it's a largish patch; I'll chop it up once it's review time.

Comment 25

[Mike de Boer [:mikedeboer]]

Attachment: Part 1: refactor PushHandler to encapsulate more of the registration flow, simplify the API and more

Comment 26

[Matthew N. [:MattN]]

Comment on attachment 8512247 [details] [diff] [review]
Part 1: refactor PushHandler to encapsulate more of the registration flow, simplify the API and more

Review of attachment 8512247 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/LoopPushHandler.jsm
@@ +339,5 @@
> +      for (let [fieldName, channelID] of fieldsToRegister) {
> +        payload[fieldName] = gChannels.get(channelID);
> +      }
> +
> +      yield MozLoopService.registerWithLoopServer(sessionType, payload);

I dislike this. As I said in bug 1079198:
"I don't think onPushRegistered should call registerWithLoopServer directly so we can properly fix bug 1076428 since right now it doesn't know whether to do a Guest or FxA registration."

No push-related methods should call registerWithLoopServer. This should be the job of MozLoopService init/setup method IMO. This push module should only know about push stuff.

Comment 27

[Matthew N. [:MattN]]

Comment on attachment 8512253 [details] [diff] [review]
Part 1: refactor PushHandler to encapsulate more of the registration flow, simplify the API and more

Review of attachment 8512253 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/LoopPushHandler.jsm
@@ +10,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Timer.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "MozLoopService",
> +                                        "resource:///modules/loop/MozLoopService.jsm");

This approach also leads to a circular dependency.

Comment 28

[Mike de Boer [:mikedeboer]]

(In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from comment #26)
> No push-related methods should call registerWithLoopServer. This should be
> the job of MozLoopService init/setup method IMO. This push module should
> only know about push stuff.

I agree; should MozLoopService then in turn ask the PushHandler for the URLs to register/ fields to pass in the request?

Comment 29

[Matthew N. [:MattN]]

Yes, or the promise returned by the successful registration can have the URLs as a resolution value.

Comment 30

[Mike de Boer [:mikedeboer]]

Attachment: Part 1: rename MozLoopPushHandler to LoopPushHandler and refresh its implementation

Comment 31

[Mike de Boer [:mikedeboer]]

To review this part 1, it might be best to take `LoopPushHandler.jsm` after you applied the patch and review the file in its entirety. Splinter might not give you the right view of what moved where.

Comment 32

[Mike de Boer [:mikedeboer]]

Attachment: Part 1: rename MozLoopPushHandler to LoopPushHandler and refresh its implementation

Unit test updates will be part 2.

Comment 33

[Matthew N. [:MattN]]

Comment on attachment 8512645 [details] [diff] [review]
Part 1: rename MozLoopPushHandler to LoopPushHandler and refresh its implementation

Review of attachment 8512645 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/LoopPushHandler.jsm
@@ +42,5 @@
> +let gInitDone = false;
> +// A flag which signifies that we've connected with the push server.
> +let gConnected = false;
> +// Maps channel IDs to their respective push end-point.
> +let gChannels = new Map();

Why make these all globals instead of "private" underscore-prefix members of the objects? This makes it harder to check the state of these variables in tests and doesn't actually hide them from extensions.

Also, while working on another bug I found that the gChannels map should IMO describe what it's holding, not what the keys are so therefore I think this should be gEndpoints.

::: browser/components/loop/LoopRooms.jsm
@@ +260,5 @@
>  Object.freeze(LoopRoomsInternal);
>  
> +const kChannelIDs = new Map([
> +  ["FxA", "6add272a-d316-477c-8335-f00f73dfde71"],
> +  ["guest", "19d3f799-a8f3-4328-9822-b7cd02765832"]

Nit: keep the case consistent. One is capitalized, while the other is not. For LOOP_SESSION_TYPE they all all-caps since they are consts.

::: browser/components/loop/MozLoopService.jsm
@@ +32,5 @@
>  Cu.import("resource://gre/modules/Task.jsm");
>  Cu.import("resource://gre/modules/FxAccountsOAuthClient.jsm");
>  
> +Cu.import("resource:///modules/loop/LoopCalls.jsm");
> +Cu.import("resource:///modules/loop/LoopRooms.jsm");

Keep these lazy so they aren't loaded when we do early returns in the initialize method.

@@ +334,5 @@
> +            gRegisteredDeferred.resolve("registered to guest status");
> +            // No need to clear the promise here, everything was good, so we don't need
> +            // to re-register.
> +          }, error => {throw error});
> +      }, error => {throw error}).catch(error => {

Can't ", error => {throw error}" be deleted? If not, you're missing a semicolon after |throw error|.

@@ +480,5 @@
>    /**
>     * Registers with the Loop server either as a guest or a FxA user.
>     *
>     * @param {LOOP_SESSION_TYPE} sessionType The type of session e.g. guest or FxA
> +   * @param {Boolean} pushUrls The push url given by the push server.

Is this intentional? The description doesn't match the type.

Comment 34

[Mike de Boer [:mikedeboer]]

Attachment: Part 1.1: rename MozLoopPushHandler to LoopPushHandler and refresh its implementation

What do you think of this?

Comment 35

[Paul Kerr [:pkerr]]

I like the move to a promise-based API. Side-effects to consider:

There are a number of xpcshell and mochi tests that rely on the ability to override the pushUrl (of which there used to be only one) for the push channels to trigger a number of test conditions. Another set of tests uses a mock version of the PushHandler that is found in both tests/xpcshell/head.js and tests/mochitest/head.js. (see bug 1074663 for the effected files.) Is there a plan to handle the broken tests?

Comment 36

[Mike de Boer [:mikedeboer]]

(In reply to Paul Kerr [:pkerr] from comment #35)
> I like the move to a promise-based API. 

\o/

> There are a number of xpcshell and mochi tests that rely on the ability to
> override the pushUrl (of which there used to be only one) for the push
> channels to trigger a number of test conditions. Another set of tests uses a
> mock version of the PushHandler that is found in both tests/xpcshell/head.js
> and tests/mochitest/head.js. (see bug 1074663 for the effected files.) Is
> there a plan to handle the broken tests?

Yeah, that'd be part 2, which I'm not really looking forward too... Part 1 should make the test bits easier to read, I hope.

Comment 37

[Paul Kerr [:pkerr]]

Comment on attachment 8512832 [details] [diff] [review]
Part 1.1: rename MozLoopPushHandler to LoopPushHandler and refresh its implementation

Review of attachment 8512832 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/LoopPushHandler.jsm
@@ +199,5 @@
> +        this._connected = true;
> +        if (this._UAID !== msg.uaid) {
> +          this._UAID = msg.uaid;
> +        }
> +        eventEmitter.emit("connected");

There seems to be a step missing here. If the UAID returned in the "hello" response is not the same as the stored version, then registration with the push server of all the channels, regardless of having been already registered, must be performed. And, if the URLs returned from the new PushServer registration have changed, registration with the LoopServer would need another cycle.

Even if the UAID is the same, any new channels that may have been passed in through a register() call should be registered once the connection has been re-established. (In the case where MozLoopService delays FxA registration until user login, for example.)

Basically, the design goal for the PushHandler module is to silently re-establish the PushServer connection across network drops, so the open socket -> hello -> register cycle must be able to re-run at any point. (see bug 1028869 for the next planned extension of the retry logic using a 30 minute ping.)

Comment 38

[Mike de Boer [:mikedeboer]]

(In reply to Paul Kerr [:pkerr] from comment #37)
> There seems to be a step missing here. If the UAID returned in the "hello"
> response is not the same as the stored version, then registration with the
> push server of all the channels, regardless of having been already
> registered, must be performed. And, if the URLs returned from the new
> PushServer registration have changed, registration with the LoopServer would
> need another cycle.

Very true! thanks for catching that! The last part of 'registration with the LoopServer would need another cycle' is also missing in the current implementation... are you trying to say that this should be added?

Comment 39

[Paul Kerr [:pkerr]]

(In reply to Mike de Boer [:mikedeboer] from comment #38)
> (In reply to Paul Kerr [:pkerr] from comment #37)
> > There seems to be a step missing here. If the UAID returned in the "hello"
> > response is not the same as the stored version, then registration with the
> > push server of all the channels, regardless of having been already
> > registered, must be performed. And, if the URLs returned from the new
> > PushServer registration have changed, registration with the LoopServer would
> > need another cycle.
> 
> Very true! thanks for catching that! The last part of 'registration with the
> LoopServer would need another cycle' is also missing in the current
> implementation... are you trying to say that this should be added?

It is something that should be done, but it is not currently the responsibility of the PushHandler. The way this is signaled is by LoopPushHandler calling the registration callback again with the new push URL. However, MozLoopService does not do anything in this case. In almost all cases I have seen in testing with the PushServer, sending a "hello" after a network disconnect, with a list of channels that have already been registered, will result in the PushServer simply accepting the old assignments. But, the PushServer is permitted to generate a fresh assignment.

Comment 40

[Matthew N. [:MattN]]

Comment on attachment 8512832 [details] [diff] [review]
Part 1.1: rename MozLoopPushHandler to LoopPushHandler and refresh its implementation

Review of attachment 8512832 [details] [diff] [review]:
-----------------------------------------------------------------

It seems like this needs significant work to address the following:

(Quoting Paul Kerr [:pkerr] from comment #37)
> Basically, the design goal for the PushHandler module is to silently
> re-establish the PushServer connection across network drops, so the open
> socket -> hello -> register cycle must be able to re-run at any point. (see
> bug 1028869 for the next planned extension of the retry logic using a 30
> minute ping.)

::: browser/components/loop/MozLoopService.jsm
@@ +324,5 @@
>      // it back to null on error.
>      let result = gRegisteredDeferred.promise;
>  
> +    // Refer to LoopCalls _and_ LoopRooms, to make sure they are loaded.
> +    LoopCalls, LoopRooms;

Why do we need this? Is there some code that runs upon import?

Comment 41

[Paul Kerr [:pkerr]]

For reference and background, here is the initial bug calling for the retry logic: bug 1002414

Comment 42

[Mike de Boer [:mikedeboer]]

Since I already made the LoopRooms class aware and deal with push notifications in bug 1089547, this bug is not needed anymore to obtain that functionality. I updated the bug fields to reflect reality.

Comment 43

[Daniel Coloma:dcoloma]

We have been checking this for FxOS Loop client and the answer format for retrieving deleted rooms is what we needed. However, we have some doubts about the specific behaviour for getting deleted rooms. When a device has received a push notification with version=x for a deleted room, what does it need to invoke to retrieve the list of deleted rooms?

 A/ GET /rooms?version=x 
 B/ GET /rooms?version=y (where y is a previous version to x and the previous version notification received by the client)

In the approach is B/, in that case, when a client registers for the first time, it needs to know the current "version" number so that it can check for deleted rooms when the first notification is received. 

Additionally, and kind of related, when multiple clients register with the same Identity, would they share "version" in the notifications received or will they have independent version counters?

Comment 44

[Mark Banner (:standard8)]

(In reply to Daniel Coloma:dcoloma from comment #43)
> We have been checking this for FxOS Loop client and the answer format for
> retrieving deleted rooms is what we needed. However, we have some doubts
> about the specific behaviour for getting deleted rooms. When a device has
> received a push notification with version=x for a deleted room, what does it
> need to invoke to retrieve the list of deleted rooms?

This is probably not the right bug to be discussing this on. I'd recommend an email thread or a separate bug.

What I would say is that from the desktop perspective, we're treating the rooms list as a cache within desktop - if the user restarts (and probably at some stage, if a reconnection happens), then we get the list afresh.

For push notifications, we always use the version provided in the notification, and that gives us the info about the deleted rooms (in the latest version of the server, the current production 0.13 doesn't).

Comment 45

[Mark Banner (:standard8)]

Support for Hello/Loop has been discontinued.

https://support.mozilla.org/kb/hello-status

Hence closing the old bugs. Thank you for your support.