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

Define scope to job queue map struct #1183

Merged
merged 1 commit into from Aug 15, 2017

Conversation

jungkees
Copy link
Collaborator

@jungkees jungkees commented Aug 9, 2017

This change clarifies a job queue isn't owned by a service worker
registration and reflects the implementations more precisely by defining
the scope to job queue map struct. This also makes it use the queue
struct defined in Infra instead of the FIFO queue concept that didn't
have a concrete reference.

Fixes #1180.

This change clarifies a job queue isn't owned by a service worker
registration and reflects the implementations more precisely by defining
the scope to job queue map struct. This also makes it use the queue
struct defined in Infra instead of the FIFO queue concept that didn't
have a concrete reference.

Fixes #1180.
@jungkees
Copy link
Collaborator Author

jungkees commented Aug 9, 2017

@jakearchibald, could you review?

/cc @wanderview

@@ -2435,7 +2437,9 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
* For *register* and *update* <a>jobs</a>, both their [=job/scope url=] and the [=job/script url=] are the same.
* For *unregister* <a>jobs</a>, their [=job/scope url=] is the same.

A <dfn id="dfn-job-queue">job queue</dfn> is a thread safe queue used to synchronize the set of concurrent <a>jobs</a>. The <a>job queue</a> contains <a>jobs</a> as its elements. The <a>job queue</a> *should* satisfy the general properties of FIFO queue. A user agent *must* maintain a separate <a>job queue</a> for each [=/service worker registration=] keyed by its [=service worker registration/scope url=]. A <a>job queue</a> is initially empty. Unless stated otherwise, the <a>job queue</a> referenced from the algorithm steps is a <a>job queue</a> for the <a>job</a>'s [=job/scope url=].
A <dfn id="dfn-job-queue">job queue</dfn> is a thread safe [=queue=] used to synchronize the set of concurrent [=jobs=]. The [=job queue=] contains [=jobs=] as its [=queue/items=]. A [=job queue=] is initially empty.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to define "thread safe" further? I'm not sure. If we do, something like "jobs must run in series"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The situation isn't clear. Spec needs thread-safeness as multiple clients share job queues. Chromium is scheduling jobs in the browser process's io thread, which means the scheduling requests are already serialized in a single thread there. I'm not sure about Gecko. https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#936 called in ServiceWorkerManager::Register is on the main thread. @wanderview, is this main thread of the browser process's main thread or each web content process's main thread? I couldn't find any IPC bridge along the call path, so presume the multi-process migration hasn't come to SW part yet? Could you explain?

Anyway, spec needs a thread concept where we can queue such jobs to serialize. There have been discussions, but it hasn't spec'ed yet. For now, i'd like to defer to those spec efforts and just leave it as "thread safe" without any additional wording. I think it'd be difficult to be clear about the order of the jobs in prose.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox, I guess tabs in FF currently run in the same process. Then, the job scheduling is serialized on the main thread of the process.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ylafon, @jakearchibald, can I merge this PR? Should I wait until I can join SW WG?

@jakearchibald jakearchibald merged commit b5ef084 into master Aug 15, 2017
@jakearchibald jakearchibald deleted the intro-scope-to-job-queue-map branch August 15, 2017 11:41
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

Successfully merging this pull request may close these issues.

None yet

2 participants