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

Should we expose reserved clients? #1216

Closed
jakearchibald opened this issue Oct 26, 2017 · 10 comments
Closed

Should we expose reserved clients? #1216

jakearchibald opened this issue Oct 26, 2017 · 10 comments
Milestone

Comments

@jakearchibald
Copy link
Contributor

Based on what we discussed in #1091:

addEventListener('fetch', event => {
  event.respondWith();

  if (!event.resultingClientId) return;

  event.waitUntil(async function () {
    const client = await clients.get(event.resultingClientId);

    // client may be a reserved client.
    // Therefore this next line may fail:
    client.navigate();
  }());
});

You'll get a real client in the weird about:blank case, otherwise you'll get a reserved client, where .navigate fails.

It's got me wondering if we shouldn't expose reserved clients at all. Instead, clients.get(resultingClientId) should wait until the client exists before resolving with a real client (or resolve undefined if the client will never exist).

This way we don't have to deal with queuing postMessage, and developers don't have to worry about what reserved clients can & can't do.

The downside is you could create a deadlock:

addEventListener('fetch', event => {
  event.respondWith(async function () {
    const client = await clients.get(event.resultingClientId);

    return new Response();
  }());
});

This would deadlock for all navigations, except in the case of the about:blank edge case. Given that deadlock here is the common scenario, it doesn't bother me too much. It'd be easy spotted during development.

@jakearchibald
Copy link
Contributor Author

@n8schloss did you have specific requirements around reserved clients? How does the above sound?

@wanderview
Copy link
Member

Couple thoughts:

  1. When would get(reservedId) resolve? When the Client becomes execution ready?
  2. If the navigation fails or redirects cross origin the Client for reservedId may never become execution ready. What does get(reservedId) do in that case?

@jakearchibald
Copy link
Contributor Author

When would get(reservedId) resolve? When the Client becomes execution ready?

As soon as it can be messaged/navigated. I'm sure it's more nuanced than that though.

If the navigation fails or redirects cross origin the Client for reservedId may never become execution ready. What does get(reservedId) do in that case?

Right now, client.get(doesntExist) will resolve with undefined, so that was my first thought.

It could reject if it's given a valid reserved client ID that doesn't become a client, but I think that'd be a race condition. If you wait long enough, that ID will no longer be reserved, so it'll resolve undefined.

@wanderview
Copy link
Member

As soon as it can be messaged/navigated. I'm sure it's more nuanced than that though.

In previous discussions reserved became non-reserved at execution ready time. This is at least a spec'd point in time for each global (although implementations may vary in reality of course):

https://html.spec.whatwg.org/#concept-environment-execution-ready-flag

Right now, client.get(doesntExist) will resolve with undefined, so that was my first thought.

So, delayed resolution until we detect the global for reservedId is being discarded and then we resolve null at that point?

I guess that's feasible.

@jakearchibald
Copy link
Contributor Author

So, delayed resolution until we detect the global for reservedId is being discarded

Are there cases where we don't even create a global? We definitely know when a response can't form a client, or when it's redirected cross-origin, so we can resolve null then.

@wanderview
Copy link
Member

wanderview commented Oct 26, 2017

Are there cases where we don't even create a global? We definitely know when a response can't form a client, or when it's redirected cross-origin, so we can resolve null then.

But, the service worker gets the FetchEvent.reservedId before its FetchEvent.respondWith() or fall-through possibly triggers the cross-origin redirect.

I think per the current spec we are always supposed to have a reserved Client even if the browser is "optimizing" by delaying the creation of the global. For example, right now firefox avoids creating the initial about:blank window global when it can, but I still pre-allocate a reserved Client ID for it. (Edit: although that is not reserved since the about:blank should be execution ready.)

@jakearchibald
Copy link
Contributor Author

I think we're agreeing. resultingClientId may reference a not-yet-created or even may-never-be-created global.

@jakearchibald jakearchibald added this to the Version 1 milestone Nov 2, 2017
@jakearchibald
Copy link
Contributor Author

From memory, I think we wanted reserved clients so we had somewhere to describe the details of the window the new page would appear in, in advance. However, we have that via replacesClientId, except in the dodgy about:blank case, where resultingClientId will give you those details.

I don't think we all understood how about:blank worked when we first thought about these APIs. I certainly didn't.

@jakearchibald
Copy link
Contributor Author

F2F: No objections to this in general. Avoids all the issues around reserved clients that @wanderview identified.

Action: Spec this. Close related issues in #1206.

@jungkees
Copy link
Collaborator

Reserved client won't be exposed. Spec'ing .resultingClientId and .targetClientId will be followed up in #1245 as V2 features.

jungkees added a commit that referenced this issue Jan 12, 2018
To reflect the decision to not expose reserved clients
(#1216) and to spec
.resultingClientId and .targetClientId in Nightly only, this:

* Removes
 - Client.reserved and its associated reserved state (V1, Nightly)
 - ClientQueryOptions.includeReserved (V1, Nightly)
 - FetchEvent.reservedClientId (V1)
 - FetchEvent.targetClientId (V1)
 - FetchEventInit.reservedClientId (V1)
 - FetchEventInit.targetClientId (V1)

* Changes
 - FetchEvent.reservedClientId to FetchEvent.resultingClientId (Nightly)
 - FetchEventInit.reservedClientId to FetchEventInit.resultingClientId
   (Nightly)
 - Handle Fetch to set FetchEvent.clientId for a navigation request to
   the empty string (V1)

* Corrects
 - matchedClients with clientObjects in Clients.matchAll() (V1, Nightly)

Related issue: #1245.

This also cleans up sort condition steps in Clients.matchAll() that
fixes #1080 (V1, Nightly)

(Changes for the Clients interface's methods will be addressed as
separate PRs.)
jungkees added a commit to jungkees/web-platform-tests that referenced this issue Jan 13, 2018
As per the decision in w3c/ServiceWorker#1216.

Related spec PR: w3c/ServiceWorker#1259.
jungkees added a commit that referenced this issue Jan 13, 2018
To reflect the decision to not expose reserved clients
(#1216) and to spec
.resultingClientId and .targetClientId in Nightly only, this:

* Removes
 - Client.reserved and its associated reserved state (V1, Nightly)
 - ClientQueryOptions.includeReserved (V1, Nightly)
 - FetchEvent.reservedClientId (V1)
 - FetchEvent.targetClientId (V1)
 - FetchEventInit.reservedClientId (V1)
 - FetchEventInit.targetClientId (V1)

* Changes
 - FetchEvent.reservedClientId to FetchEvent.resultingClientId (Nightly)
 - FetchEventInit.reservedClientId to FetchEventInit.resultingClientId
   (Nightly)
 - Handle Fetch to set FetchEvent.clientId for a navigation request to
   the empty string (V1)

* Corrects
 - matchedClients with clientObjects in Clients.matchAll() (V1, Nightly)

Related issue: #1245.

This also cleans up sort condition steps in Clients.matchAll() that
fixes #1080 (V1, Nightly)

(Changes for the Clients interface's methods will be addressed as
separate PRs.)
jungkees pushed a commit that referenced this issue Jun 13, 2018
To reflect the decision that we don't expose reserved clients (#1216),
this change introduces the client's discarded flag, and uses that flag and the
execution ready flag to resolve the promise when the client reaches one of those
states instead of capturing and exposing the reserved client.

Follow-up to 07e9487 (#1259).
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

3 participants