Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Guarding data posts against closing the tab #72

Closed
jakearchibald opened this issue Apr 1, 2015 · 16 comments
Closed

Guarding data posts against closing the tab #72

jakearchibald opened this issue Apr 1, 2015 · 16 comments

Comments

@jakearchibald
Copy link
Collaborator

Consider this:

addToOutbox(importantEmail);

fetch(url, {
  method: 'POST',
  body: importantEmail
}).then(response => {
  if (!response.ok) throw Error(response.statusText);
  removeFromOutbox(importantEmail);
  showSuccessUI();
}).catch(function() {
  return navigator.serviceWorker.ready.then(reg => {
    return reg.sync.register({tag: 'outbox'});
  });
});

If sending an email fails, we request a sync and handle it later. However, if the tab/browser closes before the request completes, or the page navigates away, the request fails but we never get the chance to send that request.

Data isn't lost, but we can't send that email in the background. It becomes more attractive to do this:

addToOutbox(importantEmail);

navigator.serviceWorker.ready.then(reg => {
  return reg.sync.register({tag: 'outbox'})
});

fetch(url, {
  method: 'POST',
  body: importantEmail
}).then(response => {
  if (!response.ok) throw Error(response.statusText);
  removeFromOutbox(importantEmail);
  showSuccessUI();

  if (outboxEmpty) {
    navigator.serviceWorker.ready.then(reg => {
      return reg.sync.getRegistration('outbox');
    }).then(syncReg => syncReg.unregister());
  }
});

As in, we schedule a sync with every important POST, and unregister it if it succeeds. I'm worried there may be a race here where the sync event fires file the fetch is still being attempted, resulting in a double fetch.

I could schedule a sync event and do all of my outbox emptying there (not from the page at all), but I have no guarantee that it'll happen immediately, and communicating success/failure back to the page is a bit of a postMessage mess.

Is there anything we can do to improve this? Should we?

Couple of half-baked ideas:

reg.sync.register({
  tag: 'outbox',
  onlySyncIfThisPromiseFailsToFulfill: fetch(url, {
    method: 'POST',
    body: importantEmail
  }).then(response => {
    if (!response.ok) throw Error(response.statusText);
  })
});

Or:

reg.sync.register({
  tag: 'outbox',
  onlySyncIfNoPagesAreOpen: true
});

…which would prevent race conditions with the page.

@annevk
Copy link
Contributor

annevk commented Apr 1, 2015

One of the UIs I suggested for one-off was prompting the user when the application is closed to allow for synchronization when the user goes online next.

Basically you can request one-off anytime as it's also a healthy way to not have to ping the network every-so-often until you find you're online again. The UI only comes up when the application is terminated.

@jakearchibald
Copy link
Collaborator Author

Are you saying sync events would never fire while the related service worker had clients?

@annevk
Copy link
Contributor

annevk commented Apr 1, 2015

No, I'm saying that as long as it has clients there's no need to display UI for one-off. Only at the point all clients go away.

@jakearchibald
Copy link
Collaborator Author

Ahh ok. I don't think that solves the issue as you could still get the race between the fetch from the page & the one in the sync listener.

@annevk
Copy link
Contributor

annevk commented Apr 1, 2015

Maybe we should change the semantics of the one-off to fire as quickly as possible when the user is online. That way you can do everything through the one-off.

@jakearchibald
Copy link
Collaborator Author

Yeah, although communicating success/failure back to the page isn't ideal. Maybe:

addToOutbox(importantEmail);

reg.sync.register({tag: 'outbox'}).then(syncReg => {
  syncReg.syncNow().then(val => {
    console.log(val); // "All done!"
  });
});

Over in the service worker:

self.addEventListener('sync', event => {
  event.waitUntil(
    fetch(url, {
      method: 'POST',
      body: getOutbox()
    }).then(r => r.json()).then(data => {
      if (data.ok) return "All done!";
      throw Error(data.err || "Unknown error");
    })
  );
});

@jakearchibald
Copy link
Collaborator Author

A little more detail: a one-off wouldn't fire as quickly as possible when the user is online, although .syncNow would trigger an immediate sync, and would resolve/reject with a structured clone of the resolved/rejected value.

@jakearchibald
Copy link
Collaborator Author

A big problem here is that errors aren't structured-cloneable. Filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=28389

@jkarlin
Copy link
Collaborator

jkarlin commented Apr 2, 2015

Given that a one-shot can only be registered when the page is open, why wouldn't it be fired right away?

@annevk
Copy link
Contributor

annevk commented Apr 3, 2015

@jkarlin it would, unless you're offline.

@KenjiBaheux
Copy link
Collaborator

@jakearchibald

would the following setup take care of the race condition?

  • a shared outbox used by both the SW and the page
  • SW/Page should move the message they want to send from the outbox to their respective plates
  • SW/Page should try sending the message they took from the outbox
  • if sending fails, SW/Page should try to put the message back into the outbox (or empty their plate at the next opportunity)
  • if sending succeeds, SW/Page should clean their plate. SW might want to let the page knows that the message was delivered.

Hmm, there might be some instances where the page is gone AND has a message sitting on its plate though... Could the SW, after it's done emptying the outbox, check for the absence of clients and take care of the Page's plate without running into a race condition?

@jakearchibald
Copy link
Collaborator Author

Hmm, there might be some instances where the page is gone AND has a message sitting on its plate though

Yeah, I'm nervous about anything that 'removes' something from an outbox before it's successfully sent.

@jakearchibald
Copy link
Collaborator Author

So, I encountered this issue when adding background-sync to https://jakearchibald-gcm.appspot.com/chat/

  1. User hits send, UI shows "sending"
  2. I add the message to an outbox in IDB [code]
  3. "sync" event - I pull the messages out of the outbox and send them [code]
  4. On completion/failure, I postMessage to all clients, telling them what happened [code]
  5. The page picks up the message and changes the UI accordingly [code]

I'm aware of all the horrible race conditions around my use of the outbox, which can be fixed by using IDB properly. However, the real painful bit is the messaging between SW and page.

I'm currently broadcasting to all clients after syncing, because I don't have a way to identify the client that initiated the sync. I need to do something on the page to check it's expecting the message it gets (I'm not doing that right now).

I'm still keen on something like this:

reg.sync.register({tag: 'outbox'}).then(syncReg => {
  return syncReg.done;
}).then(_ => {
  // sync is complete
});

Where syncReg.done fulfills once the sync successfully completes, and rejects if it somehow gives up entirely.

It'd be nice to be able to pass a value back, but that seems a lot more complicated.

@jkarlin
Copy link
Collaborator

jkarlin commented Jun 12, 2015

+1, syncReg.done seems useful

@clelland
Copy link
Contributor

I like that (obviously on one-shot syncs only; I'm not sure what the periodic equivalent would be).

It enables some interesting use cases -- it makes a 'background sync status page' easy to implement in Javascript, and makes it much easier for multiple client windows to display UI on pending syncs, even if they weren't the one to register it.

jkarlin added a commit that referenced this issue Jun 19, 2015
@jkarlin
Copy link
Collaborator

jkarlin commented Oct 8, 2015

done is in the spec, closing as fixed.

@jkarlin jkarlin closed this as completed Oct 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants