994151 - Loop desktop client should wait to have a valid generated call url to start the SimplePush registration

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Alexis Metaireau (:alexis)]

In order to not overwhelm the Simple Push server with connections that would not be useful, we should start the registration on the Simple Push server in a lazy way.

Registering the simple push server can be done only when a previous call URL was generated and is still valid, otherwise there is no need to do that registration step, so we can avoid traffic on the Simple Push server.

Comment 1

[Mark Banner (:standard8)]

We should wait for bug 976109 before doing this, as the architecture there will influence what we do here.

Comment 2

[Maire Reavy [:mreavy]]

Alexis - Do we need this for MLP?  Can you ask the Push folks if they need this done before we land it in Nightly?  Thanks.

Comment 3

[Alexis Metaireau (:alexis)]

That depends the amount of load we would generate in nightly, how many users to we expect?

SP had been specified to handle 1M concurrent connected clients, but the less we send there the less we will need to handle.

Comment 4

[Maire Reavy [:mreavy]]

I've heard that 10K users on Nightly is a safe assumption (i.e. we'll very likely have less -- especially at the beginning).  RT -- Do we have a better estimated for Alexis (based on metrics)?

Comment 5

[Mark Banner (:standard8)]

The specific issue here is that in our current state *all* nightly users will be registering with the push server (via websockets) on startup. They will also be registering with the loop server.

If we implement this bug, then we'll reduce from the number of nightly users to however many users try it out at least once.

Comment 6

[Romain Testard [:RT]]

Just so I understand, if we implement that bug a user is registered on both push and Loop servers so long as they generate a URL and for the expiration duration of the URL or forever? If the browser is closed and re-opened the user is re-registered?

Comment 7

[Maire Reavy [:mreavy]]

This should really be more like a P2.  The server piece needs us to land this for MLP.

Comment 8

[Maire Reavy [:mreavy]]

We need this so that every Nightly user doesn't register with the Loop server when MLP lands.

Comment 9

[Mark Banner (:standard8)]

Attachment: Part 1. Handle registration errors better, with simple notifications to the user, and handle subsequent attempts to register with the server

This is the first step to allowing us to delay the registration. It implements an ensureRegistered API on mozLoop, which gets called before asking the server for call-urls. If we are not registered (as in the patch with the commented out auto-registration lines), then it attempts to go and register, calling back up the tree when complete, and then allowing the call-url to be obtained.

If registration fails for any reason, the failure gets passed back up the call tree via the callbacks, so at least the user gets notified about it when they next try to generate a url.

The second part (still to be written) will handle only registering if necessary (if a call-url has been requested recently), and delaying the initial registration after startup a little bit longer.

I'm currently looking at some xpshell tests for MozLoopService, hence putting up for feedback whilst I try and do those.

Comment 10

[Mark Banner (:standard8)]

Attachment: Part 1. Ensure the client is registered before requesting a call url

This is the shared part of the changes necessary for checking registration when requesting the call-url.

Comment 11

[Mark Banner (:standard8)]

This is larger than I originally expected, due to the additional complexity of needing to sort out the failure routes.

Comment 12

[Mark Banner (:standard8)]

Attachment: Part 2 Only register with the servers if the user has obtained call urls that haven't expired yet

This is the second part, that makes us only register with the servers if the user has a call url that hasn't expired yet. Call urls could have different expiry times, so we store the latest expiry time.

Docs, tidy-up & tests still to come.

Comment 13

[Mark Banner (:standard8)]

Attachment: Part 2 (loop-client) Only register with the servers if the user has obtained call urls that haven't expired yet

loop-client part.

Comment 14

[Mark Banner (:standard8)]

Attachment: Part 1. Handle registration errors better, with simple notifications to the user, and handle subsequent attempts to register with the server. v2

Updated part 1 to include some xpcshell tests for the registration process. For this we mock the websocket interface (there's no apparent pre-written xpcshell helpers for websocket), and we use a http server to driver the loop server part of the interface.

The split of testing types seems a bit uneven, but seems the simplest ways to the individual parts.

There's also some doc cleanup in this patch.

I think this is real close to review worthy now, but I want to check over the code one more time for cases I should be testing.

Comment 15

[Mark Banner (:standard8)]

Attachment: Part 2 Only register with the servers if the user has obtained call urls that haven't expired yet. v2

Updated for bitrot

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8423844 [details] [diff] [review]
Part 1. Ensure the client is registered before requesting a call url

Looks good.  One tweak: there are two places where the jsdoc declares a callback signature with two args, but the actual code only calls back with one arg.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8424866 [details] [diff] [review]
Part 2 Only register with the servers if the user has obtained call urls that haven't expired yet. v2

Seems reasonable.  A couple of nits:

The "log" logCallUrlExpiry seems confusing.  Perhaps "noteCallUrlExpiry".

It would also be nice to name variable names and functions and log statments involving times to include the units, in order to help prevent future coding thinkos.

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

I would suggest using separate functions for converting between time units.  As it stands, the code in various functions in jumping across levels of abstractions, which makes it harder to read.

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Mark Banner (:standard8) from comment #9)
>
> If registration fails for any reason, the failure gets passed back up the
> call tree via the callbacks, so at least the user gets notified about it
> when they next try to generate a url.

Presumably, this wants a spin-off bug for MVP with better UX for the failures.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8424865 [details] [diff] [review]
Part 1. Handle registration errors better, with simple notifications to the user, and handle subsequent attempts to register with the server. v2

I _think_ this is OK, but the callback chains are getting a little complicated to keep in my head at once, which always concerns me.  One thing that's making stuff harder is that sometimes "register" or "registration" is used without a qualifier (eg "push" or "loopXhr"), so that it's easy to misread when one is new to the code.  If you somehow make the two processes more clearly distinct, perhaps just by always using a qualifier, I think that would help.

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

(I'm mildly tempted to want to use promises here to improve readability, but with none of us having had experience doing that in a Gecko context, I'm not convinced that now's the time for that).

Comment 22

[Mark Banner (:standard8)]

(In reply to Dan Mosedale (:dmose) from comment #19)
> (In reply to Mark Banner (:standard8) from comment #9)
> >
> > If registration fails for any reason, the failure gets passed back up the
> > call tree via the callbacks, so at least the user gets notified about it
> > when they next try to generate a url.
> 
> Presumably, this wants a spin-off bug for MVP with better UX for the
> failures.

Yep, bug 1013248.

(In reply to Dan Mosedale (:dmose) from comment #21)
> (I'm mildly tempted to want to use promises here to improve readability, but
> with none of us having had experience doing that in a Gecko context, I'm not
> convinced that now's the time for that).

Agreed, now isn't the time for that, and my additional thought is we may be able to re-use some of the existing push notification stuff to harden up the push service (bug 1013248).

Comment 23

[Mark Banner (:standard8)]

Attachment: [checked in] Part 1. Handle registration errors better, with simple notifications to the user, and handle subsequent attempts to register with the server. v3

Updated to address the various comments and fix a few more things. This is currently based against privs-landing, but I'll rebase it against loop-service once review is complete. I'll attach the loop-client part in a moment.

Part 2 is still in work - I want to see if I can write some sensible unit tests for it.

Comment 24

[Mark Banner (:standard8)]

Attachment: [checked in] Part 1. Ensure the client is registered before requesting a call url. v2

Comment 25

[Mark Banner (:standard8)]

Attachment: Part 2. Only register with the servers if the user has obtained call urls that haven't expired yet.

Now with tests, fixed the previous comments as well.

Comment 26

[Mark Banner (:standard8)]

Attachment: Part 2 (loop-client). Only register with the servers if the user has obtained call urls that haven't expired yet

Comment 27

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8425453 [details] [diff] [review]
[checked in] Part 1. Ensure the client is registered before requesting a call url. v2

r=dmose; one tweak suggested on github branch.

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8425450 [details] [diff] [review]
[checked in] Part 1. Handle registration errors better, with simple notifications to the user, and handle subsequent attempts to register with the server. v3

r=dmose with the comments in github addressed.

Comment 29

[Mark Banner (:standard8)]

(In reply to Dan Mosedale (:dmose) from comment #28)
> Comment on attachment 8425450 [details] [diff] [review]
> Part 1. Handle registration errors better, with simple notifications to the
> user, and handle subsequent attempts to register with the server. v3
> 
> r=dmose with the comments in github addressed.

FTR comments here: https://github.com/adamroach/gecko-dev/commit/32eca6e455bae72bb9a9afc4244acc9e51ff8fef

(In reply to Dan Mosedale (:dmose) from comment #27)
> Comment on attachment 8425453 [details] [diff] [review]
> Part 1. Ensure the client is registered before requesting a call url. v2
> 
> r=dmose; one tweak suggested on github branch.

FTR comments here: https://github.com/mozilla/loop-client/commit/bf8860f4bc43782c81dbbdc166a9ffad72d7ce91

Comment 30

[Mark Banner (:standard8)]

Comment on attachment 8425450 [details] [diff] [review]
[checked in] Part 1. Handle registration errors better, with simple notifications to the user, and handle subsequent attempts to register with the server. v3

Landed on our integration branch:

https://github.com/adamroach/gecko-dev/commit/27ff9e42ac5af2c3ca118148a678d61586290929

Comment 31

[Mark Banner (:standard8)]

Comment on attachment 8425453 [details] [diff] [review]
[checked in] Part 1. Ensure the client is registered before requesting a call url. v2

Landed in loop-client:

https://github.com/mozilla/loop-client/commit/5d3bb4ba83e0eb1d56d9b970e60560a655eb97bf

Comment 32

[Mark Banner (:standard8)]

Attachment: Part 2. Only register with the servers if the user has obtained call urls that haven't expired yet. v2

Comment 33

[Mark Banner (:standard8)]

Attachment: Part 2 (loop-client). Only register with the servers if the user has obtained call urls that haven't expired yet. v2

Updated for landing & rebasing of part 1.

Comment 34

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8427689 [details] [review]
Part 2 (loop-client). Only register with the servers if the user has obtained call urls that haven't expired yet. v2

r=dmose, with nits noted in PR addressed.

Comment 35

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8427688 [details] [review]
Part 2. Only register with the servers if the user has obtained call urls that haven't expired yet. v2

r=dmose w/the comments from the github PR addressed as you see fit.  Thanks for the hard work!

Comment 36

[Mark Banner (:standard8)]

Landed on loop-service our integration branch, closing for tracking, will comment further when this is landed in mercurial:

https://github.com/adamroach/gecko-dev/commit/5f93f71be036e11a544d6985c8ba056b35fd837e
https://github.com/mozilla/loop-client/commit/26d70718f4508b4d4689d6f74c585b3f615fe82b

Comment 37

[Mark Banner (:standard8)]

https://hg.mozilla.org/projects/elm/rev/6ae43138206f
https://hg.mozilla.org/projects/elm/rev/5a4dfcc254b8
https://hg.mozilla.org/projects/elm/rev/ca4e30a57731
https://hg.mozilla.org/projects/elm/rev/a7346129b92b

Comment 38

[Mark Banner (:standard8)]

https://hg.mozilla.org/mozilla-central/rev/6ae43138206f
https://hg.mozilla.org/mozilla-central/rev/5a4dfcc254b8
https://hg.mozilla.org/mozilla-central/rev/ca4e30a57731
https://hg.mozilla.org/mozilla-central/rev/a7346129b92b

Comment 39

[u279076]

Does this need automated or manual testing?

Comment 40

[Alexis Metaireau (:alexis)]

Clearing the needinfo for me as this is a client-side bug.

Comment 41

[u279076]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #39)
> Does this need automated or manual testing?

Dan, do you know the answer to this question?

Comment 42

[Dan Mosedale (:dmosedale, :dmose)]

I suspect the testing for this could be automated over the medium term, but we don't have functional tests going just yet.

It's not totally clear to me how one would manually test this.  Standard8 might have some thoughts on that...

Comment 43

[Mark Banner (:standard8)]

You can easily test this today by starting up the client, and within 5 seconds, opening the panel. If you start up with -jsconsole, you can generally see the registration being forced through.

For automated testing we will be testing this once we get our functional tests done (bug 976114) - by default they create a new profile, and hence we won't be auto-registering on startup, so getting a url will force registration & obtaining the url together.

The harder bit is probably testing auto registration works, although we have unit tests which partially cover that scenario.

Comment 44

[u279076]

(In reply to Mark Banner (:standard8) from comment #43)
> You can easily test this today by starting up the client, and within 5
> seconds, opening the panel. If you start up with -jsconsole, you can
> generally see the registration being forced through.

Can I please get clearer steps to reproduce? I tried starting Firefox with -jconsole and initiating a call in the latest Nightly but all I got in terminal output was the following:

> GMPInstallManager.simpleCheckAndInstall INFO Last check was: 11535 seconds ago, minimum seconds: 86400
> GMPInstallManager.simpleCheckAndInstall INFO Will not check for updates.

Comment 45

[u279076]

Mark, can you please answer comment 44? Sorry for not flagging you earlier.

Comment 46

[Mark Banner (:standard8)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #44)
> (In reply to Mark Banner (:standard8) from comment #43)
> > You can easily test this today by starting up the client, and within 5
> > seconds, opening the panel. If you start up with -jsconsole, you can
> > generally see the registration being forced through.
> 
> Can I please get clearer steps to reproduce? I tried starting Firefox with
> -jconsole and initiating a call in the latest Nightly but all I got in
> terminal output was the following:

So you should be seeing something like:

POST http://localhost:5000/registration [HTTP/1.1 200 OK 2ms]

(different server obviously)

Sometimes I've found that you have to toggle the "Net" button (sometimes on and off) to get the network activity to show.

Having looked at this again, I should probably extend my STR anyway:

1) Start Firefox with a new profile with -jsconsole on the command line
2) Wait 10-20 seconds, examine the console.

=> the above registration message shouldn't appear (wait 10-20 seconds; 5 seconds is the 'normal' time)

3) Open the Loop panel

=> Registration message should appear in the console, and a url is shown in the window.

4) Copy the url (this is needed since bug 1060610 landed)

5) Restart Firefox
6) Watch the console.

=> After about 5 seconds the registration message should appear.

Comment 47

[u279076]

Sorry this took so long for me to get to, it fell off my radar. The steps are not working for me. When I launch Firefox with -jconsole I get an error in Browser Console: "unrecognized command line flag -jconsole"

Comment 48

[Mark Banner (:standard8)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #47)
> Sorry this took so long for me to get to, it fell off my radar. The steps
> are not working for me. When I launch Firefox with -jconsole I get an error
> in Browser Console: "unrecognized command line flag -jconsole"

That should be "-jsconsole"

Comment 49

[u279076]

Thanks for your help Mark. I confirm your steps in comment 46.