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

Inevitable unhandled Promise rejection due to failure of prefetch #14115

Closed
uhyo opened this issue Sep 2, 2021 · 22 comments
Closed

Inevitable unhandled Promise rejection due to failure of prefetch #14115

uhyo opened this issue Sep 2, 2021 · 22 comments

Comments

@uhyo
Copy link

uhyo commented Sep 2, 2021

Bug report

TL; DR: Promise without .catch here.

What is the current behavior?

If you dynamic-import a chunk and that chunk tries to prefetch another chunk, and that prefetch fails for some reason, you get an unhandled Promise rejection reported even if you are doing proper error handling.

If the current behavior is a bug, please provide the steps to reproduce.

Here is a reproduction repo: https://github.com/uhyo/webpack-prefetch-rejection-repro

  1. Build by npx webpack.
  2. Open index.html.
  3. You see an unhandled Promise rejection reported.

スクリーンショット 2021-09-02 23 10 22

The repro includes a custom Webpack plugin (BoomPlugin) that simulates a failure of loading of accompanying resources. Of course, this is just for easily reproducing the issue; the case I actually faced involved mini-css-extract-plugin trying to load a CSS chunk accompanying main chunk.

What is of note is that all import() calls included in source code properly handles Promise rejections. The above image shows how a prefetch failure error is handled by import(...).catch but is still reported as a Promise rejection via another path.

Seems that this unhandled Promise rejection is inevitable; the rejecting Promise is created internally in Webpack runtime and there is no way of preventing that.

What is the expected behavior?

Properly handle the error internally, or provide a way to register a .catch handler to the internal Promise.

Other relevant information:
webpack version: 5.51.1
Node.js version: 14.15.0
Operating System: MacoS Mojave 10.14.6
Additional tools:

@alexander-akait
Copy link
Member

You can solve this on your application side

@uhyo
Copy link
Author

uhyo commented Sep 2, 2021

@alexander-akait I think I did the best in the application code in the repro. All application-side Promises have proper error handling.

Do you mean that you don't think this is a problem of Webpack runtime? Then I will bring a bug report to mini-css-extract-plugin side.

@uhyo
Copy link
Author

uhyo commented Sep 11, 2021

Hey, do I have a chance of second opinion?
I will wait for another week and then close this issue as not a bug and bring a issue to mini-css-extract-plugin that chunk loading should never fail on their side.

@alexander-akait
Copy link
Member

alexander-akait commented Sep 11, 2021

Properly handle the error internally, or provide a way to register a .catch handler to the internal Promise.

You already doing this using:

import("./split.js").catch((err) => {
  console.error("import failed", err)
})

I don't see any problems with logic here, if you have unhandled Promise rejection with mini-css-extract-plugin please open an issue in mini-css-extract-plugin repo and provide example, I agree you should have ability to catch

@alexander-akait
Copy link
Member

oh, I see, do you want to catch...

@alexander-akait
Copy link
Member

Interesting case, can you provide more information why you need to catch this? If something was not loaded it can break the application logic, so we prefer to throw an error always in this case

@alexander-akait
Copy link
Member

alexander-akait commented Sep 11, 2021

Maybe related webpack-contrib/mini-css-extract-plugin#802 (regarding logic - If something was not loaded it can break the application logic, so we prefer to throw an error always in this case)

@uhyo
Copy link
Author

uhyo commented Sep 12, 2021

Thank you for noticing 😃

I prepared another example that is less self-contained but is closer to actual situation. Sorry, I should have presented this from first.

https://github.com/uhyo/webpack-prefetch-rejection-repro-2

  1. Build by npx webpack.
  2. Open index.html.
  3. Use devtools to block request to src_split2_js.css.

スクリーンショット 2021-09-12 14 07 24

  1. Then reopen index.html and check console.

スクリーンショット 2021-09-12 14 08 23

  1. Despite all the effort to catch errors, Uncaught error is printed to console.

So when css files emitted by mini-css-extract-plugin failed to load, the Promise returned by import() rejects. This is totally fine and we can properly handle this through import(...).catch(...), but there is another path where rejecting Promise is passed from mini-css-extract-plugin runtime to webpack runtime leading to unhandled Promise rejection, which application code has no chance of intercepting.

The actual problem we're facing is that this unhandled rejection error is recorded in our error logging facility and we can't suppress the error (expect for letting the error happen but not sending it).

Error being thrown is fine, but inevitable unhandled Promise rejection is not great I think 😢

@alexander-akait
Copy link
Member

I will look at this in near future, sorry for delay

@alexander-akait
Copy link
Member

@uhyo Interesting case, we decided - if something cannot be loaded, it should interrupt the application (related, but not the same webpack-contrib/mini-css-extract-plugin#802). Just interesting - how it happens?

In theory you can use dirty workaround, put:

let original = __webpack_require__.f.prefetch;

__webpack_require__.f.prefetch = (...args) => {
  original(args).catch(() => {}); 
};

in index.js.

I don't have strong opinion here, on the one hand you are right - you should have ability to catch, on the other hand why, it was failed, don't think you can load it again. But there is case when you can load fallback. Can you provide information why you need this, what is the real case?

@uhyo
Copy link
Author

uhyo commented Sep 23, 2021

@alexander-akait thank you for your investigation!

Can you provide information why you need this, what is the real case?

Our use case is error logging. Failure of CSS chunk loading may happen, and we properly handle the error (we give up loading that resource and show a nice error page to the user). However, our error logging facility (namely Sentry) still logs it as “uncaught runtime error” because of the unhandled Promise rejection reported here, which still happens even though we .catch() it in our application code. We want to remove this alert since it is not a real problem.

We could set up the error logging facility so that we ignore “loading CSS chunk failed” errors, but we consider it as another dirty workaround. Recently unhandled Promise rejections are considered a sort of uncaught runtime errors; for example, Node.js changed the default setting to terminating the process as soon as an unhandled Promise rejection happens. Therefore, we want to remove the root cause of unhandled Promise rejections whenever possible.

I hope this answers your question 🥺

@alexander-akait
Copy link
Member

Sounds reasonable, marked as bug, feel free to send a fix

@webpack-bot
Copy link
Contributor

This issue had no activity for at least three months.

It's subject to automatic issue closing if there is no activity in the next 15 days.

@alexander-akait
Copy link
Member

bump

@mnpenner
Copy link
Contributor

I've got the exact same use-case as @uhyo. These errors are finding their way into my Sentry logs and spamming me.

Not really sure what causes them because all the ones I've seen, I try going to the URL and it does exist. Perhaps some race condition when I deploy a new version of the app or their browser is cancelling the request?

Regardless, a way to catch them so I can handle them better would be nice.

FWIW, here's what I see:

image

@alexander-akait
Copy link
Member

@vankop Can you look at this?

@vankop
Copy link
Member

vankop commented Mar 27, 2022

So.. if you try to do "inner" prefetch ( e.g. in case entrypoint => import(a) => import(/* webpackPrefetch: true */b)) you will get Uncaught (in promise) ChunkLoadError when you executing entrypoint and this is quite valid since chunk b was failed to prefetch and no .catch for it was executed yet..

When code execution will reach import(/* webpackPrefetch: true */b).catch(e => {}) catch will be executed with ChunkLoadingError (thats expected).

I understand that you want to deduplicate errors in Sentry (or other observability tool). However, there is scenario when prefetch fails, but chunk never was requested (no import(b) was executed)

@vankop vankop closed this as completed Mar 27, 2022
@uhyo
Copy link
Author

uhyo commented Mar 28, 2022

@vankop So I understand that prefetch failure cannot always be tied to import () calls, but closing this issue without any solution isn't quite right. 😢

Currently there is no way of handling failure of prefetch which leads to inevitable error logs on Sentry. How about adding an official way of handling prefetch errors?

@vankop
Copy link
Member

vankop commented Mar 28, 2022

Currently there is no way of handling failure of prefetch which leads to inevitable error logs on Sentry.

you can use .catch still, it will catch error only when execution will reach import()

How about adding an official way of handling prefetch errors?

this will be webpack specific syntax that we trying to avoid..

@alexander-akait
Copy link
Member

@vankop catch is not help here, because we don't rethrow an exception, try to repo above

@vankop
Copy link
Member

vankop commented Mar 28, 2022

@alexander-akait catch works fine
Screenshot 2022-03-28 at 13 24 23

index.js

console.log("Hello, world!")

import("./split.js").catch((err) => {
  console.log("import failed")
})

split.js

console.log("This is split chunk");

import(/* webpackPrefetch: true */"./split2.js").catch((error) => {
  console.log("import2 failed!");
})

split2.js

console.log("prefetched chunk")

after compile just remove src_split2_js.js chunk and run index.html

@vankop
Copy link
Member

vankop commented Mar 28, 2022

right now there is a bug that split.js also prefetch need a review fix #15393

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

No branches or pull requests

5 participants