0

I have this code in my Service Worker:

self.addEventListener('fetch', (event) => {
    event.respondWith(new Promise(async (resolve, reject) => {
        const req = event.request;
        try {
            const res = new HTTPResponse(resolve, reject);
            await chain_handlers(this._middlewares, function(fn, next) {
                return fn(req, res, next);
            });
            const method = req.method;
            const url = new URL(req.url);
            const path = normalize_url(url.pathname);
            const routes = this._routes[method];
            if (routes) {
                const match = this._parser.pick(routes, path);
                if (match.length) {
                    const [first_match] = match;
                    const fns = [...this._middlewares, ...routes[first_match.pattern]];
                    req.params = first_match.data;
                    setTimeout(function() {
                        reject('Timeout Error');
                    }, this._timeout);
                    await chain_handlers(fns, (fn, next) => {
                        return fn(req, res, next);
                    });
                    return;
                }
            }
            if (event.request.cache === 'only-if-cached' && event.request.mode !== 'same-origin') {
                return;
            }
            //request = credentials: 'include'
            fetch(event.request).then(resolve).catch(reject);
        } catch(error) {
            this._handle_error(resolve, req, error);
        }
    }));
});

And I've noticed that when the JS file is blocked by AdBlocker (uBlock) Chrome Extension I've got an error with a stack trace in dev tools:

Google Chrome Dev tools with exception and stack trace

The errors came from Analytics (OWA) that I have on my blog.

So the question is what is the purpose of reject in a fetch event? And how to properly handle Errors like this in service Workers to not get a stack trace?

EDIT: I've needed to put my whole code of the fetch even because got a lot of comments about async/await and Promise constructor, and it's not feasible to use one without the other in my case.

This doesn't changes the question.

EDIT2:

So I've minimized the code, this is my real question I don't care that I have anti-pattern a lot of libraries do things that you don't normally do I don't care. I just want to get the answer to this simple question:

How to don't get a stack trace and how and what is rejection in Promise for fetch event for? How to properly handle fetch rejection in a Service Worker fetch event?

This is simplified code:


self.addEventListener('fetch', (event) => {
  event.respondWith(new Promise((resolve, reject) => {
    fetch(event.request).then(resolve).catch(reject);
  }));
});

self.addEventListener('activate', (event) => {
  event.waitUntil(clients.claim());
});

And this is the console when AdBlock is on:

Console error with stack trace

jcubic
  • 61,973
  • 54
  • 229
  • 402
  • 3
    `fetch(...)` shouldn't wrapped in a promise constructor: [What is the explicit promise construction antipattern and how do I avoid it?](https://stackoverflow.com/questions/23803743/what-is-the-explicit-promise-construction-antipattern-and-how-do-i-avoid-it) – Yousaf Aug 24 '23 at 08:54
  • 2
    …and [never pass an `async function` as the executor to `new Promise`](https://stackoverflow.com/q/43036229/1048572)! – Bergi Aug 24 '23 at 09:08
  • @Yousaf I have a lot of code in my Promise constructor. You can check the full source code here: https://github.com/jcubic/wayne/blob/master/index.js#L374 – jcubic Aug 24 '23 at 11:44
  • @Bergi I have a lot of code in my Promise constructor. You can check the full source code here: https://github.com/jcubic/wayne/blob/master/index.js#L374 How I'm supposed to use await if I should not use async. – jcubic Aug 24 '23 at 11:45
  • 2
    You probably need zero of the code to be in the promise constructor. If you simply use an async function it'd still produce a promise. – VLAZ Aug 24 '23 at 11:47
  • @VLAZ but I will need to use async IIFE, I prefer to use the Promise constructor. – jcubic Aug 24 '23 at 11:49
  • Deleted `async` from my question, it doesn't change anything. – jcubic Aug 24 '23 at 11:51
  • @VLAZ in my real code I need a Promise constructor because I execute resolve and reject in different parts of the code at a later time. I also need async because I need to execute await and after it return, the code without async/wait will be a mess, I don't even know how to start making it without async. – jcubic Aug 24 '23 at 11:56
  • You probably never want to reject the Promise returned from fetch, *even if* you want the *Response* instance to be an error response. You are essentially simulating with the rejection what would happen if e.g. the device is offline and there was no ServiceWorker. Fetch doesn't reject for e.g. a 4xx/5xx response, which is why you have to check the status. – Jared Smith Aug 24 '23 at 12:09
  • What is `HTTPResponse`? What does its `.fetch()` method tries to actually call `fetch()` with? – Kaiido Aug 24 '23 at 12:14
  • 2
    @jcubic You absolutely *should* use an async IIFE, there's no good reason to prefer the `Promise` constructor especially when the latter does not handle async errors. – Bergi Aug 24 '23 at 12:18
  • 2
    Notice that in `_handle_error` you call `chain_handlers` without handling the promise it returns. The stack trace in your screenshot actually even tells you that! – Bergi Aug 24 '23 at 12:19
  • 2
    "*it's not feasible to use one without the other in my case*" - that's wrong, and actually it *does* change the question. – Bergi Aug 24 '23 at 12:23
  • @Bergi OK thanks for your input, I will try to use async IIFE, but I don't think that this is the reason I get a stack trace in the console. The reason is that the promise from fetch is rejected, Your suggesting about anti-pattern suggests the the error will be swallowed, so in this case the async is probably not the problem. But I can check if I get the error if I rewrite the code to use async IIFE, But the problem is that I need to have access to resolve and reject functions for later use. I can't use async IIFE alone. – jcubic Aug 24 '23 at 14:23
  • @jcubic Where do you need to use `resolve` and `reject` later? And even if you did, you should use `await new Promise((resolve, reject) => { … })` not put the `new Promise` around an `async` function. – Bergi Aug 24 '23 at 14:32
  • "*Your suggesting about anti-pattern suggests the the error will be swallowed*" - swallowed in the promise chain that was intended to be rejected but is now left unresolved, yes. But it's still usually reported as an unhandled rejection then. But I concur, solving the antipattern alone probably won't fix the error, it might even be completely unrelated. The error does not appear to come from the `fetch(event.request)` in the code that you posted. Have you followed the stack trace you got? – Bergi Aug 24 '23 at 14:36
  • @Bergi So to be clear you reverted my change and made the code really complex. I've edited the question again and provided a simple reproduction and my real question, I want answer for. Can you kindly check my edit? – jcubic Aug 24 '23 at 16:25
  • Sorry, I did not believe that the simplified code would produce the error message from your first screenshot (and suspected another bug, which there is, but it may not be source of the problem this question is asking about now). Can you please simplify further to `event.respondWith(fetch(event.request))`? Does that still cause an unhandled rejection? If yes, the problem appears to be with the browser implementation of `respondWith`, which does not mark the promise as handled. If that's the case, you can file a bug report, but not do anything about it in your code. – Bergi Aug 24 '23 at 17:43
  • 1
    @Bergi it throws an exception in the console. It works in Firefox. I will report an issue to the Chromium project. – jcubic Aug 24 '23 at 17:56

2 Answers2

0

This is not intuitive but you need to catch and ignore the promise rejection when using the Promise constructor:

self.addEventListener('fetch', (event) => {
  event.respondWith((new Promise((resolve, reject) => {
    fetch(event.request).then(resolve).catch(reject);
  })).catch(() => {}));
});

You will still get an error in the console. This is the console output:

Dev Tools Console with as just the error message

As you can see from the above screenshot there are no stack traces but the error about blocking the request is still there.

jcubic
  • 61,973
  • 54
  • 229
  • 402
  • The error about the failed HTTP request is *always* there. [You can filter it out though](https://stackoverflow.com/q/4500741/1048572). – Bergi Aug 24 '23 at 17:45
  • `.catch(() => {})` is not a good solution. As you can see, you're getting an error "*an object that was not a `Response` was passed to `respondWith()`.*" Don't do that! – Bergi Aug 24 '23 at 17:46
  • If the problem is really the implementation of `respondWith()`, a proper workaround that suppresses the unhandled rejection is `const promise = fetch(event.request); promise.catch(e => { /* https://stackoverflow.com/a/76971304 */ }); event.respondWith(promise);` – Bergi Aug 24 '23 at 17:48
  • @Bergi thanks for the tip but in my code, it's always a Response object. And I think that your arguments against async Promise constructor don't apply to my code because I wrap everything in try to catch and send a 500 error when this happens. So there are no silent errors. Note that the try..catch was not added to fix the issue with an anti-pattern. But I may add my code to CodeReview StackExchange to get more feedback and maybe a solution to not using the async Promise constructor. It works and it's not an easy refactor. – jcubic Aug 24 '23 at 18:02
  • 1
    @Bergi reported a bug to the Chromium project https://bugs.chromium.org/p/chromium/issues/detail?id=1475744 Thanks for helping debug this. – jcubic Aug 24 '23 at 18:24
  • Ping me when you ask on codereview, I'd be happy to show a refactor. You might want to include the `HttpResponse` class in the question. – Bergi Aug 24 '23 at 19:03
  • @Bergi Added a question on Code Review: [Refactor async Promise constructor](https://codereview.stackexchange.com/q/286779/2019). – jcubic Aug 30 '23 at 18:50
0

It appears the unhandled rejection is a problem with Chrome's respondWith implementation. It does handle both fulfillment and rejection of the argument promise, so it should not leave the rejected promise marked as unhandled.

You can work around this by handling (by ignoring) the rejection of the promise before passing it to respondWith:

async function handleRequest(request) {
    if (…) {
        … // do async stuff
        const result = await chain_handlers(this._middlewares);
        return result;
    }
    if (event.request.cache === 'only-if-cached' && event.request.mode !== 'same-origin') {
         return;
    }
    return fetch(request);
}
self.addEventListener('fetch', (event) => {
    const promise = handleRequest(event.request);
    promise.catch(_ => { /* https://bugs.chromium.org/p/chromium/issues/detail?id=1475744 */ }); // suppress unhandled rejection
    event.respondWith(promise);
});
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
  • I've just checked and this doesn't work, I need to return the value of catch and pass it `respondWith()` otherwise it still throws an unhandled rejection with a single stack frame. And I'm sure the service worker was reloaded. – jcubic Aug 24 '23 at 20:51
  • I did this and it still show stack trace: `self.addEventListener('fetch', (event) => { const promise = fetch(event.request); promise.catch(() => {}); event.respondWith(promise);});` Sorry comments don't have line breaks. – jcubic Aug 24 '23 at 20:54
  • @jcubic Oh. Thanks for testing! Then this is worse than I thought, and it's really that `respondWith` creates an internal promise that is rejected as well and cannot be handled in any way. The only choice is to never reject the promise at all; i.e. it is not possible to simulate a network error with a service worker. – Bergi Aug 24 '23 at 21:42