676404 - Refactor Sync send/receive command functionality into Clients engine

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Gregory Szorc [:gps]]

Currently, all the code dealing with commands is in the service module. The service module is being deprecated and we would like to move the command code elsewhere. The clients engine is the logical place for it.

This bug will track the movement of the commands code into the clients engine.

Comment 1

[Richard Newman [:rnewman]]

(In reply to comment #0)

\o/ for detailed bugs!

> The service module is being deprecated...

This isn't strictly true. There'll probably always be something like Service; the trick is just that it should have only a very tightly defined area of ownership, but due to its storied past it's been a dumping ground for functionality.

A lot of Marina's recent work has been pulling stuff out of Service, so it's been losing a lot of weight! Keep it up!

Comment 2

[Richard Newman [:rnewman]]

Two-parter, too: it would be nice if this didn't just cover prepCommand (sending), but also incoming command processing (which now lives in Service._lockedSync, and possibly is smeared elsewhere).

Comment 3

[Gregory Szorc [:gps]]

(In reply to comment #2)
> Two-parter, too: it would be nice if this didn't just cover prepCommand
> (sending), but also incoming command processing (which now lives in
> Service._lockedSync, and possibly is smeared elsewhere).

philikon and I talked about this yesterday. I'll be moving the main handler function over to Clients. However, the low-level APIs, such as wipeClient() and resetClient() are tied into Service pretty well, so they will remain there for the time being.

Comment 4

[Richard Newman [:rnewman]]

(In reply to comment #3)
> However, the low-level APIs, such as wipeClient()
> and resetClient() are tied into Service pretty well, so they will remain
> there for the time being.

Sounds good to me.

Comment 5

[Gregory Szorc [:gps]]

Attachment: Migrate Command Code to Clients Engine, v1

This is my first stab at moving command code to the clients engine.

I took the opportunity to rename some functions and to change the send command API to support single client targets (before, we only supported broadcasting). This feature will be required by push tab to device.

The patch is missing at least 1 unit test: verification that commands actually make their way to the server and propagate to other clients. This is forthcoming. (It is worth mentioning that prior to this, the commands API was not directly tested, sadly.)

I'm not asking for a review at this time because the patch isn't complete. But if anyone wants to do an informal review, I would welcome it.

Comment 6

[Gregory Szorc [:gps]]

Attachment: Migrate Command Code to Clients Engine, v2

* Fixes bug in sending to all clients not calling proper function.
* Adds unit test verifying commands propagate during sync

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 550741 [details] [diff] [review]
Migrate Command Code to Clients Engine, v1

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

Lots of nits for you :)

::: services/sync/modules/engines/clients.js
@@ +192,5 @@
> +   * A hash of valid commands that the client knows about. The key is a command
> +   * and the value is a hash containing information about the command such as
> +   * number of arguments and description.
> +   */
> +  _commands: [

This whole thing is awful. Can we align this all, maybe replace the reduce, get rid of that Iterator, add {} where appropriate, and in general make it not suck? We're breaking blame, so we might as well fix the code.

@@ +219,5 @@
> +   *
> +   * @param command Command string to send
> +   * @param args Array of arguments for command
> +   */
> +  _sendCommandToAllClients: function sendCommandToAllClients(command, args) {

Why are these _-prefixed? They seem like public API calls. Especially when the function name doesn't have the "_" :)

@@ +239,5 @@
> +
> +    let client = this._store._remoteClients[clientId];
> +    if (!client) {
> +      this._log.warning("Unknown remote client ID specified: " + clientId);
> +      return;

What's your justification for not throwing here?

@@ +242,5 @@
> +      this._log.warning("Unknown remote client ID specified: " + clientId);
> +      return;
> +    }
> +
> +    // Helper to determine if the client already has this command

Please punctuate your comments.

@@ +243,5 @@
> +      return;
> +    }
> +
> +    // Helper to determine if the client already has this command
> +    let notDupe = function(other) other.command != command ||

Please use conventional function syntax here ({ return ... }) to make Emacs happy for Philipp.

@@ +244,5 @@
> +    }
> +
> +    // Helper to determine if the client already has this command
> +    let notDupe = function(other) other.command != command ||
> +      JSON.stringify(other.args) != JSON.stringify(args);

Can we use Utils.deepEqual here instead of stringifying?

(Microbenchmark would tell you which way is better.)

@@ +252,5 @@
> +      command: command,
> +      args: args,
> +    };
> +
> +    if (client.commands == null)

{} please. (And when you do, cuddle the else.)

@@ +268,5 @@
> +
> +  /**
> +   * Check if the local client has any remote commands and perform them.
> +   *
> +   * @return False to abort sync

`false` is a keyword. No caps.

@@ +271,5 @@
> +   *
> +   * @return False to abort sync
> +   */
> +  processIncomingCommands: function processIncomingCommands() {
> +    this._notify("clients:process-commands", "", function() {

Where is _notify coming from?

@@ +338,5 @@
> +
> +    if (clientId) {
> +      this._sendCommandToClient(command, args, clientId);
> +    }
> +    else {

Please cuddle your elses.

::: services/sync/modules/service.js
@@ +1822,5 @@
>        // Clear out any server data
>        this.wipeServer(engines);
>  
>        // Only wipe the engines provided
>        if (engines)

{}.

::: services/sync/tests/unit/test_clients_engine.js
@@ +250,5 @@
> +  store.create(rec);
> +  let remoteRecord = store.createRecord(remoteId, "clients");
> +
> +  let action = "testCommand";
> +  let args = [ "foo", "bar" ];

Nit: cuddle the [], please.

@@ +311,5 @@
> +    }
> +    else {
> +      do_check_eq(newRecord.commands, undefined);
> +
> +      if (store._tracker)

{}.

Comment 8

[Gregory Szorc [:gps]]

> @@ +219,5 @@
> > +   *
> > +   * @param command Command string to send
> > +   * @param args Array of arguments for command
> > +   */
> > +  _sendCommandToAllClients: function sendCommandToAllClients(command, args) {
> 
> Why are these _-prefixed? They seem like public API calls. Especially when
> the function name doesn't have the "_" :)

They were public before. However, everything that sends commands was using prepCommand(), which I renamed to verifyAndSendCommand(). Since verification is good, I decided to make the non-verification functions non-public.

I think the only debate to have is whether verifyAndSendCommand() should be renamed SendCommand(), unroll _sendCommandToAllClients(), and rename _sendCommand() to have a more private name.
 
> @@ +239,5 @@
> > +
> > +    let client = this._store._remoteClients[clientId];
> > +    if (!client) {
> > +      this._log.warning("Unknown remote client ID specified: " + clientId);
> > +      return;
> 
> What's your justification for not throwing here?

I have none. I'll defer to someone with more expertise to recommend the proper behavior, which I think you did.

> @@ +243,5 @@
> > +      return;
> > +    }
> > +
> > +    // Helper to determine if the client already has this command
> > +    let notDupe = function(other) other.command != command ||
> 
> Please use conventional function syntax here ({ return ... }) to make Emacs
> happy for Philipp.

This was copied verbatim from Service, but I'll comply.

> @@ +244,5 @@
> > +    }
> > +
> > +    // Helper to determine if the client already has this command
> > +    let notDupe = function(other) other.command != command ||
> > +      JSON.stringify(other.args) != JSON.stringify(args);
> 
> Can we use Utils.deepEqual here instead of stringifying?

Also copied verbatim, but easy to address.

> @@ +252,5 @@
> > +      command: command,
> > +      args: args,
> > +    };
> > +
> > +    if (client.commands == null)
> 
> {} please. (And when you do, cuddle the else.)

Is that always the rule? I see plenty of exceptions of if's lacking braces throughout the tree.
 
> @@ +271,5 @@
> > +   *
> > +   * @return False to abort sync
> > +   */
> > +  processIncomingCommands: function processIncomingCommands() {
> > +    this._notify("clients:process-commands", "", function() {
> 
> Where is _notify coming from?

Magic from Engine. I scratched my head the first time too.

Comment 9

[Richard Newman [:rnewman]]

(In reply to comment #8)

> They were public before. However, everything that sends commands was using
> prepCommand(), which I renamed to verifyAndSendCommand(). Since verification
> is good, I decided to make the non-verification functions non-public.

Sounds fair; just make the names agree.

> I think the only debate to have is whether verifyAndSendCommand() should be
> renamed SendCommand(), unroll _sendCommandToAllClients(), and rename
> _sendCommand() to have a more private name.

That sounds about right. Would also allow for _sendCommandToAllClients to be less inefficient -- right now it does all the message construction work (e.g., stringify) over and over. Fortunately it's not a high-throughput function, so I didn't bother calling it out.

> > What's your justification for not throwing here?
> 
> I have none. I'll defer to someone with more expertise to recommend the
> proper behavior, which I think you did.

Heh. It's certainly a non-continuable condition. Just make sure the callers catch. Throw an Error object, please: Bug 675130.

> This was copied verbatim from Service, but I'll comply.

Yeah, none of these are comments about you; it's just good to fix code whenever you'll be breaking blame anyway.

> Is that always the rule? I see plenty of exceptions of if's lacking braces
> throughout the tree.

https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Control_structures

> Magic from Engine. I scratched my head the first time too.

Ah yes. Thought it was just a mispaste from Service.

Comment 10

[Gregory Szorc [:gps]]

Attachment: Migrate Command Code to Clients Engine, v3

This patch incorporates most, if not all, feedback. It also includes a few more unit tests that cover previously untested functionality.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 550821 [details] [diff] [review]
Migrate Command Code to Clients Engine, v3

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

A few things to deal with. Clearing flag.

::: services/sync/modules/engines/clients.js
@@ +197,5 @@
> +    ["resetAll",    0, "Clear temporary local data for all engines"],
> +    ["resetEngine", 1, "Clear temporary local data for engine"],
> +    ["wipeAll",     0, "Delete all client data for all engines"],
> +    ["wipeEngine",  1, "Delete all client data for engine"],
> +    ["logout",      0, "Log out client"],

Trailing comma.

@@ +198,5 @@
> +    ["resetEngine", 1, "Clear temporary local data for engine"],
> +    ["wipeAll",     0, "Delete all client data for all engines"],
> +    ["wipeEngine",  1, "Delete all client data for engine"],
> +    ["logout",      0, "Log out client"],
> +  ].reduce(function __commands(commands, entry) {

You can use destructuring here:

  ].reduce(function __commands(commands, [name, args, desc]) {
    commands[name] = {
      args: args,
      desc: desc
    };

I'd almost be tempted to just kill this code and embed:

  _commands: {
    resetAll: {
      args: 0,
      desc: "Clear temporary local data for all engines"
    },
    ...

Up to you.

@@ +207,5 @@
> +    return commands;
> +  }, {}),
> +
> +  /**
> +   * Remove any commands for the local client and mark it for upload

.

@@ +215,5 @@
> +    this._tracker.addChangedID(this.localID);
> +  },
> +
> +  /**
> +   * Sends a command+args pair to a specific client

. :)

@@ +239,5 @@
> +      command: command,
> +      args: args,
> +    };
> +
> +    if (client.commands == null) {

You can abbreviate this to

  if (client.commands) {
    ...
  }

because null and undefined are both ideal targets for this clause.

@@ +241,5 @@
> +    };
> +
> +    if (client.commands == null) {
> +      client.commands = [action];
> +    // Add the new action if there are no duplicates.

If you're commenting an 'else', either put the comment (with correct indentation) inside the clause, or break the cuddle. (That's my personal rule, at least.)

I.e., one of these two:

  if (foo) {
    // Ah, we have a foo.
    bar();
  }
  // No foo? Do something else.
  else {
    ...
  }

@@ +297,5 @@
> +
> +  /**
> +   * Validates and sends a command to a client or all clients.
> +   *
> +   * Call this does not actuallt sync the command data to the server. If the

Typo.

::: services/sync/modules/service.js
@@ +1822,5 @@
>        // Clear out any server data
>        this.wipeServer(engines);
>  
>        // Only wipe the engines provided
>        if (engines)

{}.

@@ +1828,2 @@
>        // Tell the remote machines to wipe themselves
>        else

{}.

::: services/sync/tests/unit/test_clients_engine.js
@@ +284,5 @@
> +    ["wipeEngine",  [ "tabs" ], true ],
> +    ["wipeEngine",  [],         false],
> +    ["logout",      [],         true ],
> +    ["logout",      [ "foo" ],  false],
> +    ["__UNKNOWN__", [],         false],

Trailing comma.

@@ +287,5 @@
> +    ["logout",      [ "foo" ],  false],
> +    ["__UNKNOWN__", [],         false],
> +  ];
> +
> +  for (let [action, args, expectedResult] in testCommands) {

Err, this doesn't seem like it'll be correct. Remember, `for` across an array iterates across indices, so this will bind `action` to each index, leaving the others undefined. I think you mean `for each`.

@@ +306,5 @@
> +      let command = newRecord.commands[0];
> +      do_check_eq(command.command, action);
> +      do_check_eq(command.args, args);
> +
> +      do_check_neq(store._tracker.changedIDs[remoteId], undefined);

TEST-UNEXPECTED-FAIL | /Users/rnewman/moz/hg/services-central/testing/xpcshell/head.js | TypeError: store._tracker is undefined - See following stack:

@@ +310,5 @@
> +      do_check_neq(store._tracker.changedIDs[remoteId], undefined);
> +    } else {
> +      do_check_eq(newRecord.commands, undefined);
> +
> +      if (store._tracker)

{}.

@@ +335,5 @@
> +  Clients.sendCommand(action, args, remoteId);
> +
> +  let newRecord = store._remoteClients[remoteId];
> +  do_check_eq(newRecord.commands.length, 1);
> +

Please extend this to using a command with actual arguments, and different instances of equal arguments. E.g.,

  let args1 = ["foo", {x: 2}];
  let args2 = ["foo", {x: 1}];
  ...

You're only testing the simplest case of notDupe.

@@ +351,5 @@
> +    Clients.sendCommand("wipeAll", [], id);
> +  } catch (ex) {
> +    thrown = true;
> +  }
> +

Our usual idiom is:

  let err;
  try {
    throw new Error("Whatever");
  } catch (ex) {
    err = ex;
  }

  do_check_eq(ex.message, "Whatever");

Perhaps you have some other error, and that's what's actually throwing...

Comment 12

[Gregory Szorc [:gps]]

Attachment: Migrate Command Code to Clients Engine, v4

I think I addressed everything.

Comment 13

[Richard Newman [:rnewman]]

Comment on attachment 550859 [details] [diff] [review]
Migrate Command Code to Clients Engine, v4

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

Just nits. 109 test passes. Good job.

I will fix nits and land, unless you have a pressing reason to wait.

::: services/sync/modules/engines/clients.js
@@ +308,5 @@
> +   */
> +  sendCommand: function sendCommand(command, args, clientId) {
> +    let commandData = this._commands[command];
> +    // Don't send commands that we don't know about.
> +    if (commandData == null) {

!commandData.

::: services/sync/tests/unit/test_clients_engine.js
@@ +275,5 @@
> +  let store = Clients._store;
> +
> +  let testCommands = [
> +    ["resetAll",    [],         true ],
> +    ["resetAll",    [ "foo" ],  false],

Nit: could you cuddle these?

@@ +345,5 @@
> +  newRecord.commands = [];
> +
> +  action = "resetEngine";
> +  Clients.sendCommand(action, [{ x: "foo" }], remoteId);
> +  Clients.sendCommand(action, [{ x: "bar" }], remoteId);

Additional cloned line here, please:

  Clients.sendCommand(action, [{ x: "bar" }], remoteId);

Result should still be length 2.

Comment 14

[Gregory Szorc [:gps]]

Feel free to land yourself!

Comment 15

[Richard Newman [:rnewman]]

Fixed in services:

  http://hg.mozilla.org/services/services-central/rev/3c22aef81976

Comment 16

[Richard Newman [:rnewman]]

Investigating two TPS timeouts as a result of this change.

Comment 17

[Richard Newman [:rnewman]]

Backed out until we can figure out the two TPS failures.

(This is not a failure of process; we're OK using s-c to catch this sort of thing.)

Backout:

  https://hg.mozilla.org/services/services-central/rev/92df1f6de0c6

Greg, this is reproducible on my local machine, so it should be for you. Take a look tomorrow, would you?

I will upload my with-nits-fixed patch momentarily.

Comment 18

[Richard Newman [:rnewman]]

(In reply to comment #17)

> I will upload my with-nits-fixed patch momentarily.

Rev as committed:

https://hg.mozilla.org/services/services-central/raw-rev/3c22aef81976

Greg, start there!

Comment 19

[Gregory Szorc [:gps]]

The failing TPS tests are:

* test_bug563989.js
* test_history_collision.js

Relevant log entries from bug563989:

launching firefox for phase phase4 with args ['-tps', '/tmp/tps_test_hhwfZ4', '-tpsphase', u'4', '-tpslogfile', '/home/mozauto/tps.log']
2011-08-05 17:39:26 CROSSWEAVE INFO: Weave version: 1.11.0
2011-08-05 17:39:26 CROSSWEAVE INFO: Firefox builddate: 20110804163121
2011-08-05 17:39:26 CROSSWEAVE INFO: Firefox version: 8.0a1
2011-08-05 17:39:26 CROSSWEAVE INFO: setting client.name to profile2
2011-08-05 17:39:26 CROSSWEAVE INFO: ----------event observed: sessionstore-windows-restored
2011-08-05 17:39:26 CROSSWEAVE INFO: starting action: [null]
2011-08-05 17:39:26 CROSSWEAVE INFO: executing Sync 
2011-08-05 17:39:27 CROSSWEAVE INFO: ----------event observed: weave:service:sync:error
2011-08-05 17:39:27 CROSSWEAVE INFO: sync error; retrying...
2011-08-05 17:39:27 CROSSWEAVE INFO: starting action: [null]
2011-08-05 17:39:27 CROSSWEAVE INFO: executing Sync 
2011-08-05 17:39:28 CROSSWEAVE INFO: ----------event observed: weave:service:sync:finish

1312504767289	Sync.Service	DEBUG	Exception: aborting sync, process commands said so No traceback available


And with history_collision:

launching firefox for phase phase3 with args ['-tps', '/tmp/tps_test_5GGnO2', '-tpsphase', u'3', '-tpslogfile', '/home/mozauto/tps.log']
2011-08-05 17:44:42 CROSSWEAVE INFO: Weave version: 1.11.0
2011-08-05 17:44:42 CROSSWEAVE INFO: Firefox builddate: 20110804163121
2011-08-05 17:44:42 CROSSWEAVE INFO: Firefox version: 8.0a1
2011-08-05 17:44:42 CROSSWEAVE INFO: setting client.name to profile1
2011-08-05 17:44:42 CROSSWEAVE INFO: ----------event observed: sessionstore-windows-restored
2011-08-05 17:44:42 CROSSWEAVE INFO: starting action: [null]
2011-08-05 17:44:42 CROSSWEAVE INFO: executing Sync 
2011-08-05 17:44:43 CROSSWEAVE INFO: ----------event observed: weave:service:sync:error
2011-08-05 17:44:43 CROSSWEAVE INFO: sync error; retrying...
2011-08-05 17:44:43 CROSSWEAVE INFO: starting action: [null]
2011-08-05 17:44:43 CROSSWEAVE INFO: executing Sync 
2011-08-05 17:44:44 CROSSWEAVE INFO: ----------event observed: weave:service:sync:finish

1312505083410	Sync.Service	DEBUG	Exception: aborting sync, process commands said so No traceback available


The patterns are very similar. I'm guessing we're dealing with the same bug in both.

One part of the logs that stands out to me is the "starting action: [null]". All other "starting action" entries start with null, but also have more array elements. I'll dig in.

Comment 20

[Gregory Szorc [:gps]]

Attachment: Migrate Command Code to Clients Engine, v5

There was a bug in processIncomingCommands() where the commands were cleared before obtaining the set of commands to process. There was no unit test coverage, so we didn't catch it. The bug was fixed and a unit test was added.

Additionally, Weave was not defined because main.js wasn't imported. That bug has also been squashed.

I verified that all unit and TPS tests pass with this patch.

Comment 21

[Richard Newman [:rnewman]]

Comment on attachment 551083 [details] [diff] [review]
Migrate Command Code to Clients Engine, v5

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

Ouch, I should have caught that! Still, this is why we have tests :D

Good job, gps. Will land this momentarily.

Comment 22

[Gregory Szorc [:gps]]

> Ouch, I should have caught that! Still, this is why we have tests :D

You should have caught it!? It was me who introduced the bug and foolishly had faith in the unit test coverage! I can't believe we had no unit tests around command execution (including wipe client, wipe server, and logout)! If these broke, it would be a very bad day for privacy, and pandas around the world would cry.

Comment 23

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #22)
> I can't believe we had no unit tests
> around command execution (including wipe client, wipe server, and logout)!

Welcome to the Sync team! BLAME LABS CODE. ;)

When I started, we had perhaps 50 test suites running. Now we're over 100. Philipp wrote most of those first 50 when he started. We're heading in the right direction.

Comment 24

[Richard Newman [:rnewman]]

Relanded:

  http://hg.mozilla.org/services/services-central/rev/845ed0ee7123

Had to manually apply part of the patch due to DOS EOL in test_clients_engine.js. Didn't dos2unix the whole file, because blame has value.

Comment 25

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #20)

> I verified that all unit and TPS tests pass with this patch.

Are you sure? Those emails just now report the same failures.

Comment 26

[Gregory Szorc [:gps]]

I am sure they pass. From my local machine:

TEST-PASS | test_bug563989.js

        phase1: PASS
        phase2: PASS
        phase3: PASS
        phase4: PASS
Test Summary

TEST-PASS | test_bug563989.js | 

My current tree is:

changeset:   73695:845ed0ee7123
tag:         tip
parent:      73693:92df1f6de0c6
user:        Gregory Szorc <[e-mail]>
date:        Thu Aug 04 16:19:02 2011 -0700
summary:     Bug 676404 - Migrate command APIs from Service to Clients engine. (relanded) r=rnewman

Comment 27

[Gregory Szorc [:gps]]

Also:

TEST-PASS | test_history_collision.js

        phase1: PASS
        phase2: PASS
        phase3: PASS
        phase4: PASS
Test Summary

TEST-PASS | test_history_collision.js |


I think TPS is on crack.

Comment 28

[Richard Newman [:rnewman]]

First local run:

TEST-UNEXPECTED-FAIL | test_bug563989.js | test did not complete

	phase1: PASS
	phase2: PASS
	phase3: PASS
	phase4: unknown

Running again now.

Comment 29

[Richard Newman [:rnewman]]

Turns out that

-  processCommands: function WeaveSvc_processCommands()
-    this._notify("process-commands", "", function() {

had turned into

+  processIncomingCommands: function processIncomingCommands() {
+    this._notify("clients:process-commands", "", function() {

which is rather different: note the additional "{". I missed it, and Greg didn't know the difference.

I put together a bustage fix, added a trivial test line to make sure that processIncomingCommands returns a real boolean, not undefined, and ran TPS to verify.

Pushed without review:

  https://hg.mozilla.org/services/services-central/rev/7f68a2651582

TPS looks happy. Phew.

Comment 30

[Richard Newman [:rnewman]]

Pushed to m-c:

  http://hg.mozilla.org/mozilla-central/rev/7f68a2651582

etc.