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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Does not gracefully handle AbortController with fetch #489

Open
swanson opened this issue Mar 31, 2021 · 5 comments 路 May be fixed by #490
Open

Does not gracefully handle AbortController with fetch #489

swanson opened this issue Mar 31, 2021 · 5 comments 路 May be fixed by #490
Labels
enhancement

Comments

@swanson
Copy link

swanson commented Mar 31, 2021 •
edited

馃憢

I have some fetch calls that are using AbortController (MDN) to cancel while they are in-flight. When this happens, a DomException error is raised, but generally clients want to catch it and ignore.

Since rack-mini-profiler patches fetch (see: https://github.com/MiniProfiler/rack-mini-profiler/blob/master/lib/html/includes.js#L936-L991), I can sometimes get raw Uncaught (in promise) DOMException: The user aborted a request. exceptions from the mini-profiler Javascript if I create and abort many fetches in quick succession.

I was able to make the errors go away by tacking on a catch(e => {}) to the Promise chain here, but wasn't sure if that was a good long-term fix.

Generally not a huge deal, but this will cause problems when deployed to non-development environments that have clientside JS error tracking services and was really confusing when developing to see unhandled errors (that our application was explicitly handling).

Any guidance on how to address this in the library or maybe even opt out of the fetch tracking behavior (I'd rather not have the metrics for JS request vs potential errors)?
@SamSaffron
Copy link
Member

I support both making "fetch" support optional - default on I guess so we don't break current expectations.

@swanson any chance you can contribute a PR here?

Also our patch should certainly not break semantics of fetch, if standard fetch eats this specific error then I guess we should as well?

@swanson
Copy link
Author

swanson commented Apr 7, 2021

I'm happy to help, but I'm not quite sure what the intended behavior should be @SamSaffron so any guidance would be appreciated.

It is not that standard fetch eats the error per-say, but rather that applications typically will swallow it since your code intentionally cancelled an in-flight request.

Are there any existing mechanism to report "failed" requests in the profiler? Or do they just get ignored?

Perhaps the smallest change would be to add a catch to the Promise chain and ignore specifically the DOMException: The user aborted a request.

@SamSaffron
Copy link
Member

honestly I am not sure what the right thing is to do, fetch came in as a contribution I have not used fetch that much in the wild.

Happy to start with the small change of handling DOMException: The user aborted a request.

At the moment mini profiler does not have any special mechanism for reporting failed requests, it sound like an interesting thing to add one day.

@swanson
Copy link
Author

swanson commented Apr 7, 2021

Thanks, I will take a crack at a PR and we can continue discussion there.

@swanson swanson linked a pull request Apr 7, 2021 that will close this issue
Open
@edwinv edwinv mentioned this issue Dec 1, 2023
Closed
@edwinv
Copy link

edwinv commented Dec 1, 2023

The monkey patch of fetch still doesn't handle the AbortController exceptions well. It took me quite some time to figure our why the new Turbo 8 beta was throwing these errors. Maybe we can merge the proposed PR to prevent others from having these lengthy debug sessions? Maybe @davidtaylorhq or @nateberkopec can help out? 馃檹

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Handle AbortError if fetch is cancelled in-flight swanson/rack-mini-profiler
4 participants
@SamSaffron @swanson @edwinv @nateberkopec