[service workers] Reduce reliance on global state #5206
Conversation
|
Notifying @ehsan, @mattto, and @mkruisselbrink. (Learn how reviewing works.) |
Firefox (nightly channel)Testing web-platform-tests at revision 2cdb30d All results4 tests ran/service-workers/service-worker/fetch-event-async-respond-with.https.html
/service-workers/service-worker/fetch-event-respond-with-stops-propagation.https.html
/service-workers/service-worker/fetch-request-css-base-url.https.html
/service-workers/service-worker/websocket-traffic-no-fetch.https.html
|
Chrome (unstable channel)Testing web-platform-tests at revision 2cdb30d All results4 tests ran/service-workers/service-worker/fetch-event-async-respond-with.https.html
/service-workers/service-worker/fetch-event-respond-with-stops-propagation.https.html
/service-workers/service-worker/fetch-request-css-base-url.https.html
/service-workers/service-worker/websocket-traffic-no-fetch.https.html
|
jugglinmike
force-pushed
the
sw-recording
branch
from
b0e7488 to
a1a6400
Compare
Firefox (nightly channel)Testing web-platform-tests at revision efc99c4 All results4 tests ran/service-workers/service-worker/fetch-event-async-respond-with.https.html
/service-workers/service-worker/fetch-event-respond-with-stops-propagation.https.html
/service-workers/service-worker/fetch-request-css-base-url.https.html
/service-workers/service-worker/websocket-traffic-no-fetch.https.html
|
Chrome (unstable channel)Testing web-platform-tests at revision efc99c4 All results4 tests ran/service-workers/service-worker/fetch-event-async-respond-with.https.html
/service-workers/service-worker/fetch-event-respond-with-stops-propagation.https.html
/service-workers/service-worker/fetch-request-css-base-url.https.html
/service-workers/service-worker/websocket-traffic-no-fetch.https.html
|
|
@ehsan @mkruisselbrink @mattto any thoughts on this? |
|
|
||
| self.addEventListener('fetch', function(event) { | ||
| setTimeout(function() { | ||
| try { | ||
| event.respondWith(new Response()); | ||
| result = 'FAIL: did not throw'; | ||
| ServiceWorkerRecorder.worker.save('FAIL: did not throw'); |
There was a problem hiding this comment.
here (and pretty much everywhere else) you probably want to waitUntil the result of save?
There was a problem hiding this comment.
Definitely. This was an issue in multiple places, but correcting it in this
file in particular required a slightly more involved change.
| get.onsuccess = function() { | ||
| var cursor = get.result; | ||
| if (!cursor) { | ||
| Promise.all(deleteRequests) |
There was a problem hiding this comment.
nit: I wonder if Promise.all is really the behavior you want here in case one of the delete requests rejects (Promise.all doesn't wait for all promises if any of them reject)? Although in practice if any of the delete requests fail you have bigger issues anyway, so it probably doesn't matter.
There was a problem hiding this comment.
I think it is. If one of these requests fail, the test is most likely doomed.
| @@ -0,0 +1,204 @@ | |||
| 'use strict'; | |||
| self.ServiceWorkerRecorder = (function() { | |||
There was a problem hiding this comment.
Maybe some comments explaining what ServiceWorkerRecorder is and what you would use it for could be helpful? Also might want to make it explicit that despite the need to pass a "worker" to the various client methods, all the workers will record to the same database, with no way to distinguish records from different workers (or maybe it would make sense to key of "scope" as well to at least get some support for multiple recording workers?)
Also I'm not sure what the benefit is of directing all client requests through the service worker rather than just directly accessing IDB? Is that just to get a clear linearalized order of operations?
There was a problem hiding this comment.
Maybe some comments explaining what ServiceWorkerRecorder is and what you
would use it for could be helpful?
Sure thing.
Also might want to make it explicit that despite the need to pass a "worker"
to the various client methods, all the workers will record to the same
database, with no way to distinguish records from different workers (or maybe
it would make sense to key of "scope" as well to at least get some support
for multiple recording workers?)
This sounds good to me.
Also I'm not sure what the benefit is of directing all client requests
through the service worker rather than just directly accessing IDB? Is that
just to get a clear linearalized order of operations?
Directing client requests in this way allows for keying on the service worker's
location and (in light of your previous comment) scope.
| <script> | ||
| promise_test(function(t) { | ||
| var script = 'resources/fetch-event-async-respond-with-worker.js'; | ||
| var scope = 'resources/simple.html'; | ||
|
|
||
| return service_worker_unregister_and_register(t, script, scope) | ||
| .then(function(registration) { | ||
| return wait_for_state(t, registration.installing, 'activated'); | ||
| return wait_for_state(t, registration.installing, 'activated') | ||
| .then(function() { |
There was a problem hiding this comment.
Why the nested promise chains here (and in some other places)? You could just list this then up to the main promise chain. (i.e. register.then(wait for state).then(clear).then(with iframe)...)
There was a problem hiding this comment.
I've structured the chains in this way whenever one callback requires a value
that is only available in the inner function's scope. I could leak that value
to the outer scope in order to keep the chain flat, but for cases like the
registration object, that introduces a reference that may be invalidated by
a future unregister/register cycle. So although it may be a little harder to
read, I prefer nesting closures here for safety's sake.
But you're the maintainer here, so I'm happy to defer to your preference if
that isn't convincing to you :)
|
These tests are now available on w3c-test.org |
|
Thanks for the review, @mkruisselbrink! |
jugglinmike
force-pushed
the
sw-recording
branch
from
e663ea5 to
208b132
Compare
|
Due to unrelated problems with the build process, I had to rebase this branch before the automated tests would pass. My apologies about that. In case it's helpful, I've made the original version available here: |
|
@mkruisselbrink This utility would help me write some tests around client claiming at various stages of the lifecycle (see this comment in the Chromium test suite https://chromium.googlesource.com/chromium/src.git/+/92c1a4cf87397d9a63c6e1bc0bb6ecc141a0979c/third_party/WebKit/LayoutTests/http/tests/serviceworker/waiting.html#33 ). Is there anything more I can do to help this land? |
The Service Worker specification allows user agents to terminate workers at any time. This makes global state highly volatile. Tests which rely on persistence between invocations of service worker handlers are therefore subject to sporadic failure according to the browser's internal worker termination heuristic. Update tests that previously relied on global service worker state to instead store state in a persistent IndexedDB object store. Introduce a set of utility functions to simplify interaction with this object store.
|
Recent changes in |
Firefox (nightly)Testing web-platform-tests at revision 258521d All results4 tests ran/service-workers/service-worker/fetch-event-async-respond-with.https.html
/service-workers/service-worker/fetch-event-respond-with-stops-propagation.https.html
/service-workers/service-worker/fetch-request-css-base-url.https.html
/service-workers/service-worker/websocket-traffic-no-fetch.https.html
|
Sauce (safari)Testing web-platform-tests at revision 258521d All results4 tests ran/service-workers/service-worker/fetch-event-async-respond-with.https.html
/service-workers/service-worker/fetch-event-respond-with-stops-propagation.https.html
/service-workers/service-worker/fetch-request-css-base-url.https.html
/service-workers/service-worker/websocket-traffic-no-fetch.https.html
|
Chrome (unstable)Testing web-platform-tests at revision 258521d All results4 tests ran/service-workers/service-worker/fetch-event-async-respond-with.https.html
/service-workers/service-worker/fetch-event-respond-with-stops-propagation.https.html
/service-workers/service-worker/fetch-request-css-base-url.https.html
/service-workers/service-worker/websocket-traffic-no-fetch.https.html
|
Sauce (MicrosoftEdge)Testing web-platform-tests at revision 258521d All results4 tests ran/service-workers/service-worker/fetch-event-async-respond-with.https.html
/service-workers/service-worker/fetch-event-respond-with-stops-propagation.https.html
/service-workers/service-worker/fetch-request-css-base-url.https.html
/service-workers/service-worker/websocket-traffic-no-fetch.https.html
|
The Service Worker specification allows user agents to terminate workers
at any time. This makes global state highly volatile. Tests which rely
on persistence between invocations of service worker handlers are
therefore subject to sporadic failure according to the browser's
internal worker termination heuristic.
Update tests that previously relied on global service worker state to
instead store state in a persistent IndexedDB object store. Introduce a
set of utility functions to simplify interaction with this object store.
@ehsan @mkruisselbrink @mattto See
w3c/ServiceWorker#1087 for an ongoing discussion
regarding worker termination and reliance on global state (and please feel free
to weigh in with your own perspective).
An alternative to using IndexedDB would be to send event data from the worker
to the client via the
clientsinterface.
This might be simpler in some ways, but I'm reluctant to use the Service Worker
API to test the Service Worker API.
Finally, there are still more tests that rely on global state. I am submitting
this patch in order to solicit feedback on a viable workaround prior to fully
addressing the problem.