1370985 - Change getBatched() to return records directly instead of via a callback

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

[Edouard Oger [:eoger]]

This is a spin-off of bug 1210296, which allows us to land the part 1 of the patch in advance.

Comment 1

[Edouard Oger [:eoger]]

Attachment: Bug 1370985 - Change getBatched() to return records directly instead of using a callback.

Review commit: https://reviewboard.mozilla.org/r/146808/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146808/

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8875376 [details]
Bug 1370985 - Change getBatched() to return records directly instead of using a callback.

https://reviewboard.mozilla.org/r/146808/#review151076

I think this is very close, but I'd still like another look, and I'd like someone else to too - so please fix the issues and flag Thom and myself for review (IIUC, Kit is off next week)

::: services/sync/modules/engines.js:1240
(Diff revision 1)
> -      doApplyBatchAndPersistFailed.call(this);
> -      if (!resp.success) {
> -        resp.failureCode = ENGINE_DOWNLOAD_FAIL;
> -        throw resp;
> +      if (!response.success) {
> +        response.failureCode = ENGINE_DOWNLOAD_FAIL;
> +        throw response;
> +      }
> +
> +      Async.checkAppReady();

I don't think we need this check here - promiseSpinningly will throw if we are shutting down and it seems extremely unlikely the state of that would change by the time we got here. The existing sleep inside the record handler should be all we need.  But as usual, let me know if I'm missing something :)

(Note that it *might* make sense in the async patch, I just don't think we need it here)

::: services/sync/modules/engines.js:1299
(Diff revision 1)
>        if (!resp.success) {
>          resp.failureCode = ENGINE_DOWNLOAD_FAIL;
>          throw resp;
>        }
>  
> +      Async.checkAppReady();

ditto here.

::: services/sync/modules/engines.js:1833
(Diff revision 1)
> -    };
>  
>      // Any failure fetching/decrypting will just result in false
>      try {
>        this._log.trace("Trying to decrypt a record from the server..");
> -      Async.promiseSpinningly(test.get());
> +      let json = JSON.parse(Async.promiseSpinningly(test.get()))[0];

OK, I think I've finally got my head around why these changes are necessary :) So the result from get() is a string object with various properties (which we should end up killing, but not today) - but one of the properties is "obj", which does the parse. So I think this is better written as `let json = Async.promiseSpinningly(test.get()).obj[0];`

::: services/sync/tests/unit/head_http_server.js:321
(Diff revision 1)
>          }
>        } else if (start) {
>          data = data.slice(start);
>        }
> -      // Our implementation of application/newlines.
> +
> +      if (request && request.getHeader("accept") == "application/newlines") {

I still think we should skip this first condition - unless I'm wrong, this is never true in our tests. If you really want something, I'd suggest throwing instead :)

::: services/sync/tests/unit/test_collection_getBatched.js:46
(Diff revision 1)
>        throw "Some Network Error";
>      }
>      let body = recordRange(limit, offset, totalRecords);
> -    this._onProgress.call({ _data: body });
>      let response = {
> -      body,
> +      obj: body,

this test looks wrong before your patch - the response object doesn't have `body` (and removing it doesn't cause the test to fail). As mentioned above, it *does* have `obj`, but that should be the parsed representation of the body, not the raw string.

::: services/sync/tests/unit/test_collection_getBatched.js:151
(Diff revision 1)
>      batchSize,
>      lastModified,
>      interruptedAfter: 3
>    });
> -  let response = await coll.getBatched(batchSize);
> +  let { response, records } = await coll.getBatched(batchSize);
> +  checkRecordsOrder(records);

isn't the entire point of this test that records.length == 0 in the 412 case? Apparently it is now 4.

::: services/sync/tests/unit/test_collection_getBatched.js:174
(Diff revision 1)
>      batchSize,
>      lastModified,
>      throwAfter: 3
>    });
>  
> -  await Assert.rejects(coll.getBatched(batchSize), "Some Network Error");
> +  try {

why this change? Assert.rejects is what we should be using. Note however that the existing code is wrong - there's a footgun in Assert.rejects - it should be written `await Assert.rejects(coll.getBatched(batchSize), /Some Network Error/);` - passing a string doesn't check the rejection message while passing a regex does.

::: services/sync/tests/unit/test_collection_getBatched.js:183
(Diff revision 1)
>    equal(requests.length, 3);
> -  equal(records.length, 0);
>  });
> +
> +function checkRecordsOrder(records) {
> +  for (let i = 0; i < records.length; i++) {

I think we should check records.length > 0 too.

Comment 3

[Edouard Oger [:eoger]]

Comment on attachment 8875376 [details]
Bug 1370985 - Change getBatched() to return records directly instead of using a callback.

https://reviewboard.mozilla.org/r/146808/#review151076

> this test looks wrong before your patch - the response object doesn't have `body` (and removing it doesn't cause the test to fail). As mentioned above, it *does* have `obj`, but that should be the parsed representation of the body, not the raw string.

recordRange returns an Array, so there's no need to parse that.

Comment 4

[Edouard Oger [:eoger]]

Comment on attachment 8875376 [details]
Bug 1370985 - Change getBatched() to return records directly instead of using a callback.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146808/diff/1-2/

Comment 5

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8875376 [details]
Bug 1370985 - Change getBatched() to return records directly instead of using a callback.

https://reviewboard.mozilla.org/r/146808/#review151476

This isn't really a refactor of getBatched, so much as removing the recordHandler from the collection (e.g. the changes to getBatched are fairly minor).

I think there are parts of this that are good and some that are less good (I don't like that get and getBatched offer different APIs now, and I think we could still simplify the code without breaking stuff that wants recordHandler to work -- e.g. aboutsync, although I guess running it outside of nightly won't work for very much longer...), but it's probably a net win, and will enable further cleanup so I won't complain too much.

One issue that may never happen in real world code (or it may happen all the time, it's difficult for me to say), but other than that r+.

::: services/sync/modules/record.js:711
(Diff revision 2)
>    // records (or if a network error occurs).
>    async getBatched(batchSize = DEFAULT_DOWNLOAD_BATCH_SIZE) {
>      let totalLimit = Number(this.limit) || Infinity;
>      if (batchSize <= 0 || batchSize >= totalLimit) {
>        // Invalid batch sizes should arguably be an error, but they're easy to handle
>        return this.get();

Is this still okay? AFAICT the API contract of get and getBatched is, well, very different now.

I don't know how confident I'd be that nothing relies on this (esp. since it depends on the server somewhat), so if you can you should make it do the right thing instead of throwing here.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8875376 [details]
Bug 1370985 - Change getBatched() to return records directly instead of using a callback.

https://reviewboard.mozilla.org/r/146808/#review151476

> Is this still okay? AFAICT the API contract of get and getBatched is, well, very different now.
> 
> I don't know how confident I'd be that nothing relies on this (esp. since it depends on the server somewhat), so if you can you should make it do the right thing instead of throwing here.

Yeah, good catch - I think we should just throw here.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8875376 [details]
Bug 1370985 - Change getBatched() to return records directly instead of using a callback.

https://reviewboard.mozilla.org/r/146808/#review151596

Thanks Edouard, this looks great. I'll make an about-sync change is preparation for this. Please fix the issues and land next week after the merge.

::: services/sync/tests/unit/head_http_server.js:322
(Diff revision 2)
>        }
> -      // Our implementation of application/newlines.
> -      result = data.join("\n") + "\n";
>  
> +      if (request && request.getHeader("accept") == "application/newlines") {
> +        throw new Error("This server should not serve application/newlines content");

I should have mentioned this, but please also add a `this._log.error("blah")` here too - throwing an error will cause a 500 that doesn't log any details.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8875376 [details]
Bug 1370985 - Change getBatched() to return records directly instead of using a callback.

https://reviewboard.mozilla.org/r/146808/#review151600

::: commit-message-a4911:1
(Diff revision 2)
> +Bug 1370985 - Refactor getBatched(). r?markh,tcsc

Also, Thom makes a good point that this isn't so much as "refactor" as something like "change getBatched() to return records directly instead of via a callback" or similar.

Comment 9

[Edouard Oger [:eoger]]

Comment on attachment 8875376 [details]
Bug 1370985 - Change getBatched() to return records directly instead of using a callback.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146808/diff/2-3/

Comment 10

[Edouard Oger [:eoger]]

NI myself to remind me to land this after the merge

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/164fed39b4da
Change getBatched() to return records directly instead of using a callback. r=markh,tcsc

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/164fed39b4da