656513 - _lazyMap is not a function

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Richard Newman [:rnewman]]

It darn well should be.

Bookmarks sync should check for the existence of _lazyMap prior to attempting to handle items, rather than throwing for each.

_lazyMap construction should be much more robust.

Comment 1

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

I knocked out a quick patch for this.

What this does is do most of the work of lazyMap construction up-front in _syncStartup; the thinking behind this is that the work will be done a few milliseconds later, after handling the first item, so we might as well do it up-front where it's useful to throw an exception.

The alternative (which we have now) muffles the exception inside the per-item handler, and results in a chain of "_lazyMap is not a function" log messages.

This is a small change, because a larger change (e.g., to one-shot SQL) is likely to happen when we rewrite into Repositories.

Test ensures that an exception is thrown in the right place, and _lazyMap is undefined.

Comment 2

[Philipp von Weitershausen [:philikon]]

Comment on attachment 537494 [details] [diff] [review]
Proposed patch. v1

Only problem is, every single trivial sync (one where not a single bookmark is downloaded) will now call _buildGUIDMap. I don't think we want to take this perf hit.

I was actually thinking we'd do this the other way around: define _lazyMap as a static function on the BookmarksEngine.prototype (and perhaps rename to something more descriptive, e.g. _mapDupe) and make actual map object a lazy getter. If that fails to build somehow, it will fail within calling _lazyMap/_mapDupe.

Comment 3

[Richard Newman [:rnewman]]

Attachment: Part 0: allow record handler code to abort processing of incoming records. v1

Of course, previous implementation was dumb. Thanks for catching.

We discussed on IRC an implementation which roughly does what you describe.

In order to be a little more sane, I also want to be able to abort the handling of incoming records when the engine implementation decides that it should -- e.g., because we couldn't build the GUID map, and thus every reconciliation attempt will fail. That's currently impossible:

  16:49 <@rnewman> currently, recordHandler muffles all exceptions
  16:49 <@philikon> yes. that was an improvement over any kind of exception
                    aborts the whole sync
  16:49 <@philikon> i hadn't considered the lazymap case
  16:49 <@rnewman> so I have added a thrown exception with a
                   kEngineAbortIncoming code
  16:50 <@rnewman> and an aborting flag on engine, to skip later onRecord
                   handlings and batches
  16:50 <@philikon> and to not fast-forward lastSync
  16:50 <@rnewman> and rethrowing the root cause exception
  16:50 <@rnewman> which avoids bumping times
  16:50 <@philikon> right
  16:50 <@philikon> so long as it doens't abort the whole sync...
  16:50 <@philikon> which it shouldn't
  16:51 <@philikon> since we have Service._syncEngine
  16:51 <@rnewman> yeah, I'm simply not happy with how elegant this feels
  16:51 <@rnewman> 'course, that might be because I'm working with our engine
                   code :)


This patch adds an exception-based avenue for code inside the record handler to communicate up to _processIncoming, and logic to abort if this situation is detected.

Part 1 will rejig _lazyMap/_guidMap and use this code.

Marking this as feedback? rather than r?, because I want to know if you agree with the broad approach before I write tests for this individual component -- it's currently only exercised by the tests that will end up in Part 1, and I don't consider that sufficient.

Comment 4

[Richard Newman [:rnewman]]

Attachment: WIP for Part 1.

Uploading this for completeness.

Comment 5

[Philipp von Weitershausen [:philikon]]

Comment on attachment 538667 [details] [diff] [review]
Part 0: allow record handler code to abort processing of incoming records. v1

>+// Signal to the engine that processing further records is pointless.
>+kEngineAbortIncoming:                  "engine.abort.incoming",

I kinda feel like this shouldn't be a global const. It's an engine implementation detail, for engines that choose to implement applyIncoming rather than applyIncomingBatch. It should at least be called kEngineAbortApplyIncoming to indicate the limited scope where it can be used. Also, perhaps we should reflect the fact that it's an error const? eEngineAbortApplyIncoming? We're going to have lots of those once Marina (hint hint) gets to fight the War on Unknown Error.

>   applyIncomingBatch: function applyIncomingBatch(records) {
>     let failed = [];
>     for each (let record in records) {
>       try {
>         this.applyIncoming(record);
>+      } catch (ex if (ex.code == kEngineAbortIncoming)) {
>+        // Rethrow so that _processIncoming will abort.
>+        this._log.warn("Got abort exception while applying incoming records.");
>+        throw ex;
>       } catch (ex) {
>         this._log.warn("Failed to apply incoming record " + record.id);
>         this._log.warn("Encountered exception: " + Utils.exceptionStr(ex));
>         failed.push(record.id);
>       }

Rethrowing sucks because it messes up the traceback. I suggest turning around the logic. Also, why not make the string the exception itself?

       try {
         this.applyIncoming(record);
       } catch (ex if (ex != kEngineAbortApplyIncoming) {
         this._log.warn("Failed to apply incoming record " + record.id);
         this._log.warn("Encountered exception: " + Utils.exceptionStr(ex));
         failed.push(record.id);
       }

>+      if (aborting) {
>+        throw aborting.cause;
>+      }

aborting.cause? Why not just aborting?

Looks good otherwise. Tests plz :)

Comment 6

[Richard Newman [:rnewman]]

(In reply to comment #5)

> I kinda feel like this shouldn't be a global const.

Agreed.

> Rethrowing sucks because it messes up the traceback. I suggest turning
> around the logic.

Good idea.

> aborting.cause? Why not just aborting?

So that the original exception (the one which caused an engine to decide to abort) would be the one that appears in the trace. Much handier to see "Exception: invalid URI" than "Exception: aborting". Do you agree?

Comment 7

[Philipp von Weitershausen [:philikon]]

(In reply to comment #6)
> > aborting.cause? Why not just aborting?
> 
> So that the original exception (the one which caused an engine to decide to
> abort) would be the one that appears in the trace. Much handier to see
> "Exception: invalid URI" than "Exception: aborting". Do you agree?

I agree, but that contract between processIncoming and applyIncomingBatch isn't at all obvious. Actually, the more I think about it, your patch has it slightly backwards. I think it should be this way:

applyIncomingBatch can throw any kind of exception and processIncoming will use abort on it. This whole kEngineAbortIncoming business (or whatever we call it) is just a contract between applyIncomingBatch and applyIncoming, to figure out whether applyIncomingBatch should add the record to the failed list or abort.

So if applyIncoming throws something with kEngineAbortIncoming, applyIncomingBatch unpacks aborting.cause and rethrows that with the right stack (ex.stack). Sound good?

Comment 8

[Philipp von Weitershausen [:philikon]]

(In reply to comment #7)
> applyIncomingBatch can throw any kind of exception and processIncoming will
> use abort on it.

s/use//

Comment 9

[Richard Newman [:rnewman]]

Attachment: Part 0: lift SteamEngine from test_syncengine_sync.js to allow reuse. v1

Code duplication makes children cry.

Comment 10

[Richard Newman [:rnewman]]

Attachment: Part 1: implement aborting (and tests). v2

Note that _processIncoming can now throw, albeit in uncommon circumstances. The test makes this clear. I'm in two minds as to whether we should do this, which I believe means we won't continue on to _processOutgoing, or whether this should just end _processIncoming... thoughts?

Comment 11

[Richard Newman [:rnewman]]

Attachment: Part 1: implement aborting (and tests). v3

Comment 12

[Richard Newman [:rnewman]]

Attachment: Part 2: throw abort if _guidMap fails to build. v1

Comment 13

[Philipp von Weitershausen [:philikon]]

Comment on attachment 539105 [details] [diff] [review]
Part 0: lift SteamEngine from test_syncengine_sync.js to allow reuse. v1

The petrolhead in me is delighted by the whole RotaryEngine idea, but now none of my steam engine examples make any sense anymore! :( But seriously, any reason for the rename? Just makes the diff unnecessarily large.

r=me with a preference to keeping the diff small (keep the SteamEngine name)

Comment 14

[Richard Newman [:rnewman]]

(In reply to comment #13)

> r=me with a preference to keeping the diff small (keep the SteamEngine name)

As discussed on IRC:

  rnewman: reason for renaming SteamEngine to RotaryEngine when lifting into shared test code is that a lot of test files define their own, either partially or completely.
  philikon: (we do shadowing all the time btw)
  rnewman: well, sure, but names are cheap and confusion is expensive 
  philikon: yeh ok

I'd prefer to keep test code unambiguous, and "yeh ok" sounds OK to me, so unless I hear otherwise I'll land this as is.

Comment 15

[Richard Newman [:rnewman]]

Part 0:
  https://hg.mozilla.org/services/services-central/rev/d7cda0cfdab0

Comment 16

[Philipp von Weitershausen [:philikon]]

Comment on attachment 539122 [details] [diff] [review]
Part 1: implement aborting (and tests). v3

This patch is great proof of why our APIs are just completely bonkers. Begrudgingly: r=philikon

Comment 17

[Philipp von Weitershausen [:philikon]]

Comment on attachment 539123 [details] [diff] [review]
Part 2: throw abort if _guidMap fails to build. v1

>diff --git a/services/sync/modules/engines.js b/services/sync/modules/engines.js
>--- a/services/sync/modules/engines.js
>+++ b/services/sync/modules/engines.js
>@@ -208,17 +208,17 @@ Store.prototype = {
...

Seems like these changes should be part of Part 1, and in fact they already are... Are you sure you uploaded the most recent patch? :)


>-    // Lazily create a mapping of folder titles and separator positions to GUID
>-    this.__defineGetter__("_lazyMap", function() {
>-      delete this._lazyMap;
>+    // Clean up after last sync, in case _syncFinish didn't.
>+    delete this._guidMap;

Why wouldn't _syncFinish clean up? Perhaps we should wrap _processIncoming in a try-finally clause to ensure the clean up no matter what? r=me with that.

Comment 18

[Richard Newman [:rnewman]]

> Seems like these changes should be part of Part 1, and in fact they already
> are... Are you sure you uploaded the most recent patch? :)

Apparently not! :D

I shifted all that stuff, so that this part doesn't touch engines.js at all.

> Why wouldn't _syncFinish clean up? Perhaps we should wrap _processIncoming
> in a try-finally clause to ensure the clean up no matter what? r=me with
> that.

I was just carrying the logic across from the previous implementation, and adding an explanatory comment. _syncFinish isn't always run; if _uploadOutgoing or _processIncoming fails, for example.

We can't kill _guidMap at the end of _processIncoming: it's also needed for _uploadOutgoing.

The right solution is for the bookmarks engine to implement _syncCleanup, so that's where I'm putting the code.

Comment 19

[Richard Newman [:rnewman]]

Attachment: Part 2: throw abort if _guidMap fails to build. v2

This version implements _syncCleanup, which is inside a try..finally block higher up the stack. It's clean, too :)

Comment 20

[Philipp von Weitershausen [:philikon]]

Comment on attachment 539256 [details] [diff] [review]
Part 2: throw abort if _guidMap fails to build. v2

rnewman++ for discovering hooks that I implemented many many moons ago :)

Comment 21

[Richard Newman [:rnewman]]

Pushed:

Part 1: https://hg.mozilla.org/services/services-central/rev/769eb1758cda
Part 2: https://hg.mozilla.org/services/services-central/rev/bb9de4e7d77a

Comment 22

[Philipp von Weitershausen [:philikon]]

http://hg.mozilla.org/mozilla-central/rev/769eb1758cda
http://hg.mozilla.org/mozilla-central/rev/bb9de4e7d77a