Fix for uncatchable AbortErrors

[karolyi]

Patching `window.fetch` without handling the `AbortError` results in
JS code that uses fetch, becoming unable to catch these errors. As such,
application errors will be emitted to the console (and in the case of
webpack-dev-server, to the browser window), that are very mysterious.
Even if one has try/catch blocks within an async function, or then/catch
blocks for the Promise using `fetch()`, these errors will not be
catched.

I am not fully in the know as to why it works this way (the internals of
an async/Promise are very convoluted especially when `this` is involved
as in here), but I suspect the patched call will emit its own
`AbortError` extra to the one that comes from the original one, and the
uncatched one in the patched function will resurface even after catching
one of them.

[karolyi]

A hint as to what might be going on here, as these guys are also struggling with the same problem:

MiniProfiler/rack-mini-profiler#490 (comment)

[karolyi]

Demonstrably, the issue could be fixed with returning the chained promise, not the original one, i.e.:

        const origFetch = window.fetch;
        window.fetch = function () {
            const promise = origFetch.apply(this, arguments);
            return promise.then(function (response) {
                if (response.headers.get("djdt-store-id") !== null) {
                    handleAjaxResponse(response.headers.get("djdt-store-id"));
                }
                // Don't resolve the response via .json(). Instead
                // continue to return it to allow the caller to consume as needed.
                return response;
            });
            // return promise;
        };

The "forking" theory is right. The current implementation adds a chain to the promise for which the errors aren't handled here, and can't be handled from user code (since that chain is never returned, unlike above).

For me it really doesn't matter which solution we take, but we have to take one ASAP because this can be very confusing for devs like me. I've spent about 6 hours to investigate this issue.

[matthiask]

Thank you! I've read through the linked thread. I think we should either catch and silence all errors in our request or return the chained promise -- silencing only AbortError seems a little brittle to me. What do you think?

I agree that this is something we should fix relatively quickly. I wonder if there's a way to construct a testcase for this, or at least document an easy way to reproduce the problem and the fix?

[karolyi]

Just my 2cents again: I'd return the new chain instead of handling the errors on the "forked" chain. That is, my latter version.

My reason for it is: the real source of the errors are hard to find. I have a pretty complex setup with webpack, babel and typescript on the frontend — and that's only the part that might have been involved in this — add to that the toolbar's rebinding of window.fetch(). When the chain exploded (I've aborted a long running fetch), the trace of the error didn't point back to the toolbar. I've tried it both with chromium and firefox, the traceback of the unhandled exception in the console wasn't helpful anywhere.

Me spending 6 hours of investigating this issue was spent with turning babel off, then using Promises instead of async functions to check tsc output, and looking into devtools debug, and trying to change things around in my own code. Neither of these resulted in pointing out the rebound fetch.

Only the linked issue and searching around led me to simply echo the fetch function into the console, which then can be clicked and points exactly to the toolbar code with the broken function:

If you simply return the chained result, you then don't create a 'forked' chain: even if exceptions come from your addition, they can probably be traced back to the debug toolbar more easily.

As for the test case, I don't think that's possible, lest you're willing to build a test case with using headless browsers and checking console output on aborted requests with catched errors. Since I don't have the time for that, maybe the second best possibility would be to write a well formulated comment to the fixed code (linked with this issue), so others wouldn't run into the same problem when toolbar decides to change things around again.

I can issue another quick pull request for that if you like, in hopes of a quick merge and timely release.

[matthiask]

I can issue another quick pull request for that if you like, in hopes of a quick merge and timely release.

That would be wonderful. Thanks in advance!

[karolyi]

One pull request, coming right up.