Bg sync replay fix #601

prateekbh · 2017-06-06T11:08:30Z

Fixes #597

After this PR if during the entire replay process any request fails, we'll be passing a rejected promise to event.waitUntil of sync event, so that browser can replay the failed requests

jeffposnick

There are a few things to address from the inline comments, but a larger question relates to tests: is there an existing test that should have caught the bug in the original behavior? If that test already exists, and wasn't catching the bug, that sounds like it needs to be updated. If there wasn't a test for this behavior at all, then it would be ideal if you could add in a test to accompany this PR that will confirm that things work as expected.

jeffposnick · 2017-06-06T14:14:50Z

packages/workbox-background-sync/src/lib/request-manager.js

@@ -42,49 +42,58 @@ class RequestManager {
  }

  /**
+   * function to play one single request
+   * given its hash
+   * @param {String=} hash


Are you using {String=} to indicate that this is an optional parameter? If so, we've been using [hash] elsewhere in the docs (the first option listed in http://usejsdoc.org/tags-param.html#optional-parameters-and-default-values)

nah, its not an optional comment, removed the =

jeffposnick · 2017-06-06T14:15:29Z

packages/workbox-background-sync/src/lib/request-manager.js

+   * function to play one single request
+   * given its hash
+   * @param {String=} hash
+   * @return {Promise}


Can you provide some context about what the promise represents? I.e. "Resolves when..., rejects when..."

jeffposnick · 2017-06-06T14:17:24Z

packages/workbox-background-sync/src/lib/request-manager.js

+          response: response.clone(),
+          idbQDb: this._queue.idbQDb,
+        });
+        this._globalCallbacks.onResponse


Can you switch to if(this._globalCallbacks.onResponse) {...}. We've been using that syntax rather than && elsewhere in the project.

jeffposnick · 2017-06-06T14:18:52Z

packages/workbox-background-sync/src/lib/request-manager.js

+          && this._globalCallbacks.onResponse(hash, response);
+      }
+    } catch(err) {
+      this._globalCallbacks.onRetryFailure


Similar to the previous comment about preferring if() to &&.

jeffposnick · 2017-06-06T14:19:50Z

packages/workbox-background-sync/src/lib/request-manager.js

+        idbRequestObject: reqData.request});
+      const response = await fetch(request);
+      if(!response.ok) {
+        return Promise.reject(response.status);


Just to confirm: you're not invoking the this._globalCallbacks.onRetryFailure callback here. Is that intentional?

fixed this, moved this to parent function to catch all kind of rejections at one place

jeffposnick · 2017-06-06T14:29:12Z

packages/workbox-background-sync/src/lib/request-manager.js

+        await this.replayRequest(hash);
+      } catch (err) {
+        failedItems.push(err);
+        failedItems++;


I'm not clear as to why you'd increment an array. Is this a typo?

sorry, yeah typo

jeffposnick · 2017-06-06T14:31:12Z

packages/workbox-background-sync/src/lib/request-manager.js

-    }, Promise.resolve());
+  async replayRequests() {
+    let failedItems = [];
+    for (let hash of this._queue.queue) {


Does this._globalCallbacks.onRetryFailure end up adding in the failed entries to the end of this._queue.queue? If so, it seems like you may end up processing the same entry more than once while iterating through this._queue.queue if one of the earlier replay requests fails.

I am a little unclear as of what do you mean by end up adding in failed entries?

The values in this._queue.queue are set somewhere—I'm not sure where. I'm asking whether they're set inside of this._globalCallbacks.onRetryFailure, and if they are, how they're being set.

What I'm worried about is the potential for an infinite loop while you iterate over this._queue.queue. I could imagine this happening if you're adding a new entry to the end of this._queue.queue every time there's a failure inside the loop.

oh ok I guess what you are asking, so this._globalCallbacks.onRetryFailure is actually just a callback from user land so that we can notify the user that hey request X has been failed,

user can then show a notification, do a console log or anything he/she wish for. This has nothing to do with requests getting stored, that portion is completely de-coupled from this.

bg-sync calls this._globalCallbacks.onRetryFailure every-time it fails replaying the request and calls this._globalCallbacks.onResponse whenever we successfully fetch a request

So what's the code that removes items from this._queue.queue? Don't you have to remove them within one of the callbacks? And not remove them if there's a failure?

Shouldn't the request remain in the queue if it is failed, so that it can be replayed at later point?
And for the removal part, I remove them with a separate . cleanupQueue method, which is also auto-called while initialization.

jeffposnick · 2017-06-06T14:31:55Z

packages/workbox-background-sync/src/lib/request-manager.js

   * function to start playing requests
   * in sequence
-   * @return {void}
+   * @return {Promise}


Can you provide some context as to what this Promise represents?

gauntface · 2017-06-06T19:14:49Z

packages/workbox-background-sync/src/lib/request-manager.js

+        failedItems++;
+      }
+    }
+    return failedItems.length > 0 ?


Since this is an async function, can you just throw an error?

I am not sure about this, so rejecting the promise like this gives me an opportunity to pass the entire array of failes Requests and their causes. just in case developer decides to do something with it.

gauntface · 2017-06-06T19:15:51Z

My only comment on this PR, this feels like a fairly big change, without any test changes or new tests to come with it.

It sounds like something was broken, which meant it wasn't covered by a test, is there anyway we can add a test for it?

prateekbh · 2017-06-07T04:30:40Z

@jeffposnick @gauntface yeah this code never had a test backing it up, I will be adding new tests for this change.

anilanar · 2017-06-07T11:50:43Z

packages/workbox-background-sync/src/lib/request-manager.js

-  replayRequests() {
-    return this._queue.queue.reduce((promise, hash) => {
+  async replayRequests() {
+    let allRequestsStatus = [];


This can be a const actually.

prateekbh · 2017-06-07T14:34:39Z

@jeffposnick @gauntface the new test for failing promise is added

anilanar · 2017-06-07T15:07:56Z

I wonder... Can we get confirmation from Chrome team about this? @addyosmani On 7. June 2017 at 16:34:41, Prateek Bhatnagar (notifications@github.com<mailto:notifications@github.com>) wrote: @jeffposnick<https://github.com/jeffposnick> @gauntface<https://github.com/gauntface> the new test for failing promise is added — You are receiving this because you commented. Reply to this email directly, view it on GitHub<#601 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAuASDAKnUd9bJay2kqxQpkOqV1eCC-Hks5sBrUBgaJpZM4NxMLE>.

prateekbh · 2017-06-07T15:13:47Z

@anilanar confirmation about?

anilanar · 2017-06-07T15:16:13Z

About what does passing a rejected promise do to the sync event. That blog post I shared in the original issue is too old, perhaps things have changed since then?

prateekbh · 2017-06-07T15:17:28Z

@anilanar i just confirmed it is working like that, will re confirm before submitting

prateekbh · 2017-06-07T18:06:31Z

packages/workbox-background-sync/src/lib/request-manager.js

+  async replayRequest(hash) {
+    try {
+      const reqData = await this._queue.getRequestFromQueue({hash});
+      if(reqData.response) {


@jeffposnick this prevents replaying an already "successfully" played request

jeffposnick · 2017-06-07T20:15:23Z

The code changes look 👍

There are various CI test failures. I think some of them have to do with node no longer being happy with throw 'Some string';. You now need to do throw Error('Some string');

gauntface · 2017-06-08T16:27:27Z

packages/workbox-background-sync/src/lib/request-manager.js

        });
-    }, Promise.resolve());
+        if (this._globalCallbacks.onResponse)


Not to be address in this PR, but I can't help but feel like this should be an event that is emitted rather than a callback function.

gauntface · 2017-06-08T16:27:57Z

packages/workbox-background-sync/src/lib/request-manager.js

+          this._globalCallbacks.onResponse(hash, response);
+      }
+    } catch(err) {
+      return Promise.reject(err.stack);


Why are you returning just the stack here? Why not the whole error which will include the message.

gauntface · 2017-06-08T16:29:02Z

packages/workbox-background-sync/src/lib/request-manager.js

+  }
+
+  /**
+   * function to start playing requests in sequence


Could you beef up this message up a little as it'll be in the reference docs.

i.e. When you need to replay all of the currently queued requests, call this method, it'll do XYZ, ....

I tried adding some description though this one is internally called. The one available to the user to call is in background-sync-queue

gauntface · 2017-06-08T16:30:26Z

packages/workbox-background-sync/src/lib/request-manager.js

+   * @return {Promise} Resolves if all requests are played successfully,
+   * rejects if any of the request fails during the replay
+   *
+   * @memberOf RequestManager


This member of isn't needed for individual functions. The function is part of the class and whatever the class is a memberof is the important bit.

All of this is unnecessary because you've marked this class as private.

gauntface · 2017-06-08T16:32:29Z

packages/workbox-background-sync/test/browser/background-sync-queue.js

@@ -71,17 +74,29 @@ describe('background sync queue test', () => {

  it('check push proxy', async () => {
    const currentLen = backgroundSyncQueue._queue.queue.length;
-    await backgroundSyncQueue.pushIntoQueue({request: new Request('http://lipsum.com')});
+    await backgroundSyncQueue.pushIntoQueue({request: new Request('http://localhost:3000/__echo/counter')});
+    await backgroundSyncQueue.replayRequests();


Why are you replaying requests here but not before? My hunch from the code is that you are only checking the queue length, so whats the point of replaying?

cleaned this up, made bgSyncQueue to be reinitiated before every test, so that replay is not needed anymore.

gauntface · 2017-06-08T16:33:34Z

packages/workbox-background-sync/test/browser/background-sync-queue.js

@@ -71,17 +74,29 @@ describe('background sync queue test', () => {

  it('check push proxy', async () => {
    const currentLen = backgroundSyncQueue._queue.queue.length;
-    await backgroundSyncQueue.pushIntoQueue({request: new Request('http://lipsum.com')});
+    await backgroundSyncQueue.pushIntoQueue({request: new Request('http://localhost:3000/__echo/counter')});
+    await backgroundSyncQueue.replayRequests();
    chai.assert.equal(backgroundSyncQueue._queue.queue.length, currentLen + 1);


The 'currentLen + 1' always concerned me a bit here, is this test reliant on other tests passing (i.e. if one test is incorrect and the length is wrong or currentLen is not updated, will this test fail?

cleaned this up, made bgSyncQueue to be reinitiated before every test, so that no test is interdependent on each other

gauntface · 2017-06-08T16:35:14Z

packages/workbox-background-sync/test/browser/background-sync-queue.js

    await backgroundSyncQueue.replayRequests();
    chai.assert.equal(responseAchieved, 2);
  });

+  it('check replay failure with rejected promise', async function() {
+    await backgroundSyncQueue.pushIntoQueue({request: new Request('http://localhost:3000/__echo/counter')});
+    await backgroundSyncQueue.pushIntoQueue({request: new Request('http://localhost:3002/__echo/counter')});


It might be better to have an end point that just always fails. It's not clear from this test that you are referencing a failing URL.

Would '/__test/404/' endpoint work for your needs?

done, thaks its much cleaner now :)

gauntface

There are a few testing comments I've raised that would be good to address, specifically the invalid URL

addyosmani · 2017-06-09T05:00:13Z

Let's aim to address Matt's feedback and make sure Travis is happy before we merge this.

gauntface · 2017-06-09T19:29:28Z

packages/workbox-background-sync/test/browser/background-sync-queue.js

@@ -15,6 +15,10 @@
 /* global chai, workbox */

 'use strict';
+const testServerGen = require('../../../../utils/test-server-generator.js');


This test makes no sense I'm afraid.

test files under **test/browser/ are unit tests that run in the browser. There is no require, there is no need to start a server and this code just fails :(

sorry, for the goof up, I managed to fix all the test cases. lets sync up on this sometime today

gauntface · 2017-06-12T16:40:53Z

packages/workbox-background-sync/test/browser/background-sync-queue.js

-    = new workbox.backgroundSync.test.BackgroundSyncQueue({
+  let backgroundSyncQueue;
+
+  beforeEach(async ()=>{


Please use function() {} here. This is because mocha does some weird binding stuff that means fat arrows have unexpected results if you tried to do this.timeout or this.retries.

gauntface · 2017-06-12T16:41:23Z

packages/workbox-background-sync/test/browser/background-sync-queue.js

      callbacks: CALLBACKS,
    });
+  });
+
+  afterEach(async ()=> {


Same as above - switch to function.

gauntface

Two last comments and then I'm happy - sorry Prateek.

prateekbh · 2017-06-13T18:57:21Z

packages/workbox-background-sync/src/lib/request-queue.js

@@ -140,7 +140,8 @@ class RequestQueue {
    assert.isType({hash}, 'string');

    if(this._queue.includes(hash)) {
-      return await this._idbQDb.get(hash);
+      const req = await this._idbQDb.get(hash);


@gauntface just wanted to discuss this small change.
actually, this small change fixed everything failing on firefox, do you have any context?

nope nothing from me. cc @jeffposnick as FYI

prateekbh · 2017-06-19T02:55:27Z

@jeffposnick can you approve if all looks good?

prateekbh added 2 commits June 5, 2017 21:59

reject promise if replay fails

774e941

sending rejectied promise to sync event if replay fails

bfbd46d

prateekbh requested a review from jeffposnick June 6, 2017 11:08

updating chai for async functions

93c1b24

jeffposnick suggested changes Jun 6, 2017

View reviewed changes

gauntface reviewed Jun 6, 2017

View reviewed changes

prateekbh added 2 commits June 7, 2017 11:12

fixing suggested changes

dc316e1

fixing existing tests

583837f

prateekbh force-pushed the bgSyncReplayFix branch 2 times, most recently from 84bc46d to 7a58572 Compare June 7, 2017 10:17

anilanar reviewed Jun 7, 2017

View reviewed changes

adding test for promise rejection

1f9f5bf

prateekbh force-pushed the bgSyncReplayFix branch from 7a58572 to 1f9f5bf Compare June 7, 2017 13:46

prateekbh changed the title ~~WIP: Bg sync replay fix~~ Bg sync replay fix Jun 7, 2017

prateekbh commented Jun 7, 2017

View reviewed changes

gauntface reviewed Jun 8, 2017

View reviewed changes

gauntface suggested changes Jun 8, 2017

View reviewed changes

fixing tests

129f4f5

prateekbh force-pushed the bgSyncReplayFix branch 3 times, most recently from 21f41aa to 028f335 Compare June 9, 2017 09:25

fixing log errors

280e3cb

prateekbh force-pushed the bgSyncReplayFix branch from 028f335 to 280e3cb Compare June 9, 2017 09:26

gauntface suggested changes Jun 9, 2017

View reviewed changes

prateekbh force-pushed the bgSyncReplayFix branch 6 times, most recently from 5036599 to fc1e741 Compare June 12, 2017 09:30

gauntface reviewed Jun 12, 2017

View reviewed changes

gauntface suggested changes Jun 12, 2017

View reviewed changes

fixing browser tests for bgSync

8a66894

prateekbh force-pushed the bgSyncReplayFix branch from fc1e741 to 8a66894 Compare June 12, 2017 16:46

prateekbh commented Jun 13, 2017

View reviewed changes

gauntface approved these changes Jun 13, 2017

View reviewed changes

jeffposnick approved these changes Jun 19, 2017

View reviewed changes

prateekbh merged commit 8012213 into master Jun 19, 2017

prateekbh deleted the bgSyncReplayFix branch June 19, 2017 16:02

Bg sync replay fix #601

Bg sync replay fix #601

Conversation

prateekbh commented Jun 6, 2017

jeffposnick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauntface commented Jun 6, 2017

prateekbh commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateekbh commented Jun 7, 2017

anilanar commented Jun 7, 2017 via email

prateekbh commented Jun 7, 2017

anilanar commented Jun 7, 2017

prateekbh commented Jun 7, 2017

prateekbh Jun 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffposnick commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauntface left a comment

Choose a reason for hiding this comment

addyosmani commented Jun 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauntface left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateekbh commented Jun 19, 2017

prateekbh Jun 7, 2017 •

edited

Loading