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

clarify behavior when fetch event handler throws #896

Closed
wanderview opened this Issue May 10, 2016 · 23 comments

Comments

Projects
None yet
10 participants
@wanderview
Copy link
Member

wanderview commented May 10, 2016

Currently the spec seems clear that this should not trigger a network error:

addEventListener('fetch', function(evt) {
  throw('boom');
});

I think it says this should trigger a network error, though:

addEventListener('fetch', function(evt) {
  evt.respondWith(new Response('hmm'));
  throw('boom');
});

Because step 18 of Handle Fetch says to set handleFetchFailed to true if the task is discarded. And then we don't enter step 20 because respondWithEntered is true. So we should enter step 21 and trigger a network error.

(Note, because the response passed to respondWith() is coerced to a Promise by webidl the exception will by thrown before respondWith() is fulfilled with the Response.)

Talking with @jakearchibald, though, it seems we should just accept the value passed to respondWith() in this case. This is what chrome currently does.

If this is the desired behavior we should fix the spec to handle that case. We probably need a separate flag from handleFetchFailed to note synchronous event handler failures.

Firefox currently treats all of these examples as a network error. I'm going to write a bug to fix that.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented May 10, 2016

@wanderview wanderview added this to the Version 1 milestone May 10, 2016

@annevk

This comment has been minimized.

Copy link
Member

annevk commented May 10, 2016

Why would a task be discarded here?

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented May 10, 2016

I thought that was part of step 18.6.2 "Abort these steps". If that doesn't cause the task to be discarded, what does?

@annevk

This comment has been minimized.

Copy link
Member

annevk commented May 10, 2016

Oh wait what. Why does this monkey patch event dispatch in this horrible way? This should just dispatch the event and then do something with the result. Not do something on a per-listener basis. That's broken.

@jakearchibald

This comment has been minimized.

Copy link
Collaborator

jakearchibald commented Jul 27, 2016

Pre F2F notes: Decide what we want to do. Hopefully something that fits with how events are dispatched elsewhere.

@jakearchibald

This comment has been minimized.

Copy link
Collaborator

jakearchibald commented Jul 28, 2016

F2F:

  • If the fetch event throws, the browser default will continue to happen - this is how other browser events work
  • But what about the install event? There are tests that suggest that throwing in here should fail install, but that's not how events work
  • We're going to stick with how events work
@jakearchibald

This comment has been minimized.

Copy link
Collaborator

jakearchibald commented Jul 28, 2016

  • TODO: find out about the Chromium test that suggests otherwise
@inian

This comment has been minimized.

Copy link

inian commented Jul 28, 2016

@jakearchibald so what happens if there is an exception in the install event?

@mkruisselbrink

This comment has been minimized.

Copy link
Collaborator

mkruisselbrink commented Jul 29, 2016

Currently in chrome (and in an earlier spec) throwing an exception in the install event would cause the install to fail (this got added in https://bugs.chromium.org/p/chromium/issues/detail?id=425960). But since no other event in the web platform behaves in a way where exceptions thrown from event handlers influence further handling of that event this seems a bit of an odd "exception". So the suggestion is to do what the current spec says, and not fail installation when an install event handler throws.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Aug 1, 2016

I think n@8schloss said they use thrown exceptions in install events to block install, no?

I think the install event case would be a lot easier if there was a "yes please install" method like respondWith(). Currently install is do-something-by-default, while fetch event is do-nothing-by-default. The do-nothing-by-default is much easier to match other event behavior.

@jakearchibald

This comment has been minimized.

Copy link
Collaborator

jakearchibald commented Aug 3, 2016

@n8schloss are you using throw outside of waitUntil in the install event?

@n8schloss

This comment has been minimized.

Copy link

n8schloss commented Aug 16, 2016

@jakearchibald, Nope, we're throwing in the waitUntil if we want to block the install.

@jungkees

This comment has been minimized.

Copy link
Collaborator

jungkees commented Aug 16, 2016

Currently install is do-something-by-default, while fetch event is do-nothing-by-default.

I think fetch is also do-something-by-default: respond to the client after all regardless the fetch event handler gave a response to it or not.

But I agree it was an oversight that we had made throw lead to the installation failure. @n8schloss, would it still make sense to change the behavior of install to not fail upon throw in install event's handlers?

@n8schloss are you using throw outside of waitUntil in the install event?

@jakearchibald, I don't think I understood your point here. Can you explain what your point was in this question?

@n8schloss

This comment has been minimized.

Copy link

n8schloss commented Aug 16, 2016

@jungkees, I think that if you're rejecting the waitUntil promise or throwing somewhere in the waitUntil that the install really should fail. At Facebook, we currently rely on this to ensure that we don't get conflicting versions of the service worker during a Facebook code push, but more generally, I think that something going wrong during SW instillation is a good reason to not finish the install. If you're saving a bunch of stuff to indexedDB as part of your instillation and then it randomly throws during install the average developer is probably not thinking about this use case when building their service worker and likely would just want the install event to happen again later (either the next time the SW tries to update or by registering the SW again).

@NekR

This comment has been minimized.

Copy link

NekR commented Aug 16, 2016

@n8schloss

I think that if you're rejecting the waitUntil promise or throwing somewhere in the waitUntil that the install really should fail.

It's not about rejecting passed to waitUntil() promise, but about having runtime error / throw error in event handler itself (same tick). e.g. this:

self.addEventListener('install', (e) => {
   __UNDEFINED_1_.doSomething(); // this blocks install

  e.waitUntil(...);

   __UNDEFINED_2_.doSomething(); // this blocks install
});

This is tough question if it should. Personally I think it's should because it isn't a UI event which should just ignore things are continue delivering normal UI to the user. It's rather an important event of the important thing like SW which ideally must have 0 errors, because if they are they could affect whole website, all users, etc.

So personally I think that any (uncaught) error in install's event handler should prevent installation.

@jungkees

This comment has been minimized.

Copy link
Collaborator

jungkees commented Aug 17, 2016

I think that if you're rejecting the waitUntil promise or throwing somewhere in the waitUntil that the install really should fail.

As @NekR explained, the current spec satisfies this.

Now I understand why @jakearchibald asked this:

@n8schloss are you using throw outside of waitUntil in the install event?

So, the left question is sorting out the behavior of throw in the event handler. This is between maintaining consistency with other events and making the install not allow any error. Would like to hear more opinions.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Aug 17, 2016

How would you even go about detecting those execptions? Change event dispatch? If service workers needed that kind of behavior it should have created its own eventing system.

@jungkees

This comment has been minimized.

Copy link
Collaborator

jungkees commented Aug 17, 2016

Right. We removed the monkey patch (that tried to detect exceptions) as you commented before. If we'd really want this, I think we might have to change event dispatch. That is, we'll need a new flag in an event like exception thrown flag and to set it in https://dom.spec.whatwg.org/#concept-event-listener-inner-invoke step 2.7 when the callback throws an exception. And SW just uses that flag later in the install steps.

But I slightly prefer we stay with the current event behavior for consistency, having clarified that any error occurring in async tasks will make the install fail.

@slightlyoff

This comment has been minimized.

Copy link
Contributor

slightlyoff commented Aug 23, 2016

I'm fine with the resolution @jungkees proposes: staying with the current behavior for consistency.

@annevk: why the frustration? Many spec authors need to integrate with DOM. That's just how it goes. Events are a core bit of the platform and they do need to change sometimes. Not here, which seems good, but telling folks to go away and invent a parallel mechanism isn't helpful.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Aug 23, 2016

Just to clarify "current behavior" here means what the spec currently says, but not what browsers currently implement, right? We're saying that throwing synchronously in the install event handler will have no functional impact, right?

That works for me.

@jungkees

This comment has been minimized.

Copy link
Collaborator

jungkees commented Aug 23, 2016

We're saying that throwing synchronously in the install event handler will have no functional impact, right?

That's right. So I think we agree on this current spec behavior.

Closing.

@jungkees jungkees closed this Aug 23, 2016

@delapuente

This comment has been minimized.

Copy link

delapuente commented Aug 23, 2016

But will throwing inside .waitUntil() make install to fail?

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Aug 23, 2016

But will throwing inside .waitUntil() make install to fail?

As long as it triggers a promise rejection, yes.

I guess we should clarify what this does:

void broken() {
  return new Promise((resolve, reject) => {
    setTimeout(_ => {
      throw new Error('boom');
      resolve();
    }, 0);
  });
}
evt.waitUntil(broken());

Here, the promise never resolves or rejects. Eventually it will be killed by the browser for taking too long. I think this should probably be treated as a rejection. I'm not 100% sure what we do today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment