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

Handle AbortError if fetch is cancelled in-flight #490

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

swanson
Copy link

@swanson swanson commented Apr 7, 2021

Resolves #489

Per guidance here: https://developers.google.com/web/updates/2017/09/abortable-fetch#reacting_to_an_aborted_fetch

When you abort an async operation, the promise rejects with a DOMException named AbortError
You don't often want to show an error message if the user aborted the operation, as it isn't an "error" if you successfully do what the user asked. To avoid this, use an if-statement such as the one above to handle abort errors specifically.

Users may choose how to handle the rejected promise in their own code, but the profiler will not raise an uncaught exception.

@swanson
Copy link
Author

swanson commented Apr 7, 2021

I'm going to test this out tomorrow in my app since it doesn't seem like there are any automated tests for the Javascript code.

@SamSaffron
Copy link
Member

Maybe we re-throw instead of logging for e.name !== 'AbortError' ?

I am still somewhat confused at how calling a non patched "fetch" works given that the promise would be aborted.

@swanson
Copy link
Author

swanson commented Apr 7, 2021

This is my understanding of the situation, but I am also new to using AbortController!

In my application code, I have something like this:

export default class extends Controller {
  connect() {
    this.controller = new AbortController();
  }

  show() {
    const { signal } = this.controller;

    fetch("/some/endpoint", { signal })
      .then(r => r.text())
      .then(html => {
        // do some stuff
        console.log(html);
      })
      .catch(() => {});
  }
  
  hide() {
    this.controller.abort();
  }
}

If we hide before the AJAX request is finished, we want to abort the pending request. The fetch promise is rejected and we catch it and suppress any errors.

But the profiler has patched fetch (here) and added on an additional then chain. So it's the equivalent of

fetch("/some/endpoint", { signal })
    .then(r => r.text())
    .then(html => {
      // do some stuff
      console.log(html);
    })
    .catch(() => {})
    .then(response => {
      // do the rack-mini-profiler stuff since the response is finished
    })

Since the additional then from rack-mini-profiler is after the catch block, the error gets thrown.

@codecov-io
Copy link

codecov-io commented Apr 7, 2021

Codecov Report

Merging #490 (2ddd6d4) into master (94670b7) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #490   +/-   ##
=======================================
  Coverage   88.64%   88.64%           
=======================================
  Files          18       18           
  Lines        1250     1250           
=======================================
  Hits         1108     1108           
  Misses        142      142           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94670b7...2ddd6d4. Read the comment docs.

@SamSaffron
Copy link
Member

This is where I am getting really confused:

From my reading of this code it should be running:

fetch("/some/endpoint", { signal })
    .then(response => {
      // do the rack-mini-profiler stuff since the response is finished
    })
    .then(r => r.text())
    .then(html => {
      // do some stuff
      console.log(html);
    })
    .catch(() => {})
   

Something about the implementation in mini profiler does not feel right, maybe we should refactor this code a bit so it is clearer, this giant try catch nest is not that easy to understand.

@swanson
Copy link
Author

swanson commented Apr 7, 2021

Ah, I think you are right and maybe this https://github.com/MiniProfiler/rack-mini-profiler/blob/master/lib/html/includes.js#L983-L985 is actually where the AbortError is getting logged in my application. I will investigate more and see if I can narrow it down.

edit: I think I am now equally confused! If the request is aborted then it should not even get into the patched fetch at all! Hmm.

@swanson
Copy link
Author

swanson commented Apr 7, 2021

After more testing, I think we were both wrong. The Promise chain "forks" since RMP returns the original fetch chain, not the one that the additional "look for x-mini-profile-ids" then is attached to.

This makes sense because even when I was getting the unhandled exceptions, my app was still working correctly.

fetch  -> rack-mini-profiler chain (no exception handling)
      \
        -> application code chain (user controlled exception handling)

So whatever my application code does has no bearing on the exception handling in the profiler (and vice-versa). I think the original solution then would be best: the profiler will need to shallow the AbortError to avoid to extra noise in JS error tracker and confusion when developing.

The PR as it currently exists seems good to me and resolves my issue after testing.

@tylerwray
Copy link

Just lost 3 hours debugging this until I found that the mini-profiler monkey-patches window.fetch. Any chance of getting this merged?

@tiagotex
Copy link

tiagotex commented Oct 3, 2024

We also spent way too much time trying to figure out why this error was appearing in our error tracker. Any chance of getting this merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Does not gracefully handle AbortController with fetch
6 participants