search

MiniProfiler / rack-mini-profiler

Profiler for your development and production Ruby rack apps.
MIT License
3.7k stars 402 forks source link

Does not gracefully handle AbortController with fetch #489

Open swanson opened 3 years ago

swanson commented 3 years ago

:wave:

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

edwinv commented 9 months ago

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? 🙏