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

(Bugfix) Resolve issue where calling originalFetch can throw an unhandled rejection. #433

Merged

Conversation

Codex-
Copy link
Contributor

@Codex- Codex- commented Nov 25, 2021

This PR does the same change as #375 but I already made the change before noticing this PR and have some explanation around the fix 😅

Currently the existing code for processFetch calls originalFetch and stores the promise, it then attempts to attach a chain to the original promise to fork the promise resolution for raygun specific handling of things. This is fine except the catch is attached to the parent promise instead of the new then chain which has been added to the original promise.

This consequently means that if the newly attached then chain throws an exception it is not handled.

Original flow:

            ┌──────────────┐
            │ processFetch │
            └───────┬──────┘
                    │
                    ▼
┌─────────────────────────────────┐
│                                 │
│           ┌───────────────┐     │
│           │ originalFetch │     │
│           └───────┬───────┘     │
│                   │             │
│                   ▼             │
│              ┌─────────┐        │
│              │ promise │        │
│              └─┬──┬──┬─┘        │
│                │  │  │          │
│     ┌──────────┘  │  └──────┐   │
│     ▼             ▼         │   │
│ ┌──────┐      ┌───────┐     │   │
│ │ then │      │ catch │     │   │
│ └───┬──┘      └───────┘     │   │
│     │                       │   │
└─────┼───────────────────────┼───┘
      │                       │
   throws                  returns
      │                       │
      │                       ▼
      ▼                  ┌─────────┐
┌───────────┐            │ promise │
│ Unhandled │            └─────────┘
│           │
│ Rejection │
└───────────┘

This small change attaches the catch to the new then and handles throw errors as what appears to be the original intention of the code:

            ┌──────────────┐
            │ processFetch │
            └───────┬──────┘
                    │
                    ▼
┌─────────────────────────────────┐
│                                 │
│           ┌───────────────┐     │
│           │ originalFetch │     │
│           └───────┬───────┘     │
│                   │             │
│                   ▼             │
│              ┌─────────┐        │
│              │ promise │        │
│              └─┬─────┬─┘        │
│                │     │          │
│              ┌─┘     │          │
│              │       │          │
│              ▼       │          │
│          ┌──────┐    │          │
│          │ then │    │          │
│          └───┬──┘    │          │
│              │       │          │
│              ▼       │          │
│          ┌───────┐   │          │
│          │ catch │   │          │
│          └───────┘   │          │
│                      │          │
└──────────────────────┼──────────┘
                       │
                    returns
                       │
                       ▼
                  ┌─────────┐
                  │ promise │
                  └─────────┘

There are a number of other small oddities I noticed when tracking this down in this prototype that I will submit another PR for as they are not related to this immediate fix.

Fixes #430
Fixes fetch calls throwing outside of their calling try/catch #366

@Codex-
Copy link
Contributor Author

Codex- commented Nov 25, 2021

We're currently trying to trial Raygun as a solution and keep hitting this issue preventing us from giving it a thorough run 😅 any feedback or review is appreciated 😄

@Olwiba
Copy link
Collaborator

Olwiba commented Nov 26, 2021

Great work here @Codex-! appreciate the explanation & the diagrams 🤩
I've created a prerelease from your fork for you to test out, you can use the following snippet to make use of your changes while we go through the testing process on our end.

<script type="text/javascript">
    !function(a,b,c,d,e,f,g,h){a.RaygunObject=e,a[e]=a[e]||function(){
    (a[e].o=a[e].o||[]).push(arguments)},f=b.createElement(c),g=b.getElementsByTagName(c)[0],
    f.async=1,f.src=d,g.parentNode.insertBefore(f,g),h=a.onerror,a.onerror=function(b,c,d,f,g){
    h&&h(b,c,d,f,g),g||(g=new Error(b)),a[e].q=a[e].q||[],a[e].q.push({
    e:g})}}(window,document,"script","//cdn.raygun.io/raygun4js/prerelease/2.26.0-a/raygun.min.js","rg4js");
</script>

I've also updated your trial end date (12 Dec) to ensure you get to properly test out Raygun!

Always love seeing community submissions on our OSS & I'd love to say thank you!
Send me an email at ollie@raygun.com and we can get a goodie box sent out to you 😄

@Codex-
Copy link
Contributor Author

Codex- commented Nov 26, 2021

Great work here @Codex-! appreciate the explanation & the diagrams 🤩 I've created a prerelease from your fork for you to test out, you can use the following snippet to make use of your changes while we go through the testing process on our end.

Thanks for this, although I did a quick check over the new minified code and found the patch has not been applied 😭 :

var i = c.apply(null, arguments);
try {
    var s = { method: n, requestURL: r, baseUrl: a };
    t && t.body && (s.body = t.body),
    u.executeHandlers(u.requestHandlers, s),
        i.then(
            ...
        ),
        i.catch(
            ...
        );

Where i is that of the original variable promise.

I've also updated your trial end date (12 Dec) to ensure you get to properly test out Raygun!

Thank you, we really appreciate it.

Always love seeing community submissions on our OSS & I'd love to say thank you! Send me an email at ollie@raygun.com and we can get a goodie box sent out to you 😄

Will do 😄

@Olwiba
Copy link
Collaborator

Olwiba commented Nov 26, 2021

Apologies - try now @Codex-, I had originally built the master branch from your fork. The snippet should now target the bugfix/fetch_unhandled_rejection branch in your fork.

I also tried to build the feature/fetch_handling_improvements branch so I could upload this too but linting failed at src/raygun.network-tracking.js line 240 - 'originalFetch' is not defined.

@Codex-
Copy link
Contributor Author

Codex- commented Nov 26, 2021

Great! Thanks for doing that, i'll take another look at that PR :)

@Olwiba
Copy link
Collaborator

Olwiba commented Nov 26, 2021

You should be able to build the provider with grunt build from the root dir, this will run jshint to lint the updates

@Codex-
Copy link
Contributor Author

Codex- commented Nov 26, 2021

Yup found the grunt script and got it working, almost nostalgic using grunt again 😅, building now and some more diagrams added to that PR, let's move that discussion over there 👍

@darcythomas darcythomas self-requested a review December 8, 2021 19:34
Copy link
Contributor

@darcythomas darcythomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good, thanks :D

src/raygun.network-tracking.js Show resolved Hide resolved
@darcythomas darcythomas merged commit 496df0b into MindscapeHQ:master Dec 8, 2021
@Codex- Codex- deleted the bugfix/fetch_unhandled_rejection branch December 8, 2021 20:36
@darcythomas
Copy link
Contributor

This has now been released to production :D

@Codex-
Copy link
Contributor Author

Codex- commented Dec 13, 2021

awesome, thanks!

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

Successfully merging this pull request may close these issues.

Uncaught (in promise) TypeError cannot be caught
3 participants