10 ms·
The gotcha of unhandled promise rejections
- pyrolistical 4y agoThis seems like a bug with the unhandled rejected promise handler. IMO it should only trigger for unhandled promises that are garbage collected. This way the given example wouldn’t cause a false positive
- jaffathecake 4y agoIt would. If chapterPromises[0] rejects, then chapterPromises[1] is never handled (because it's redundant), even once it's GC'd.
- samsquire 4y agoIt's similar to manual memory management: you have to remember to do the other side of the thing you're doing. Structured concurrency is one approach to solving this problem. In a structured concurrency a promise would not go out of scope unhandled. Not sure how you would add APIs for it though in Javascript. See Python's trio nurseries idea which uses a python context manager. https://github.com/python-trio/trio https://github.com/python-trio/trio I'm working on a syntax for state machines and it could be used as a DSL for promises. It looks similar to a bash pipeline but it matches predicates similar to prolog. In theory you could wire up a tree (graph if you have joins) of structured concurrency with this DSL. https://github.com/samsquire/ideas4#558-assign-location-multinames-multiexpressions https://github.com/samsquire/ideas4#558-assign-location-mult...
- tech2 4y agoPython 3.11 introduced asyncio.TaskGroup[0] to cover the same use-case as trio nurseries (scoped await on context manager exit). It's imperfect, sure, but it improves matters. [1] https://docs.python.org/3/library/asyncio-task.html#asyncio.TaskGroup https://docs.python.org/3/library/asyncio-task.html#asyncio....
- klabb3 4y ago> It's similar to manual memory management Funny, I used this very analogy about concurrency the other day. I think we live in a manually managed concurrency era, because we haven’t entirely figured out the right mix of patterns to get to the point when it makes sense. In the future, I expect all non-low-level languages simply agrees on one or two models, just like RAII and GC solved the manual memory management problem. That, and we still have a lot of bugs from concurrency related matters which we basically have no way to test. Structured concurrency is most certainly part of the solution, but I there are a few devils in the details that are a lot of work (or careful thought) to get right.
- samsquire 4y agoThanks for your comment. I would really enjoy talking to you more about this. From what you wrote, I agree, we are indeed in an era where concurrency and its notation has yet to settle on an elegant approach. With microservices and distributed systems simultaneous independent execution and ordering and consistency then the problems of concurrency reveal themselves. Do you have any ideas how you would want to define concurrency? What notation would you like to be capable of writing? What is your favourite representation of concurrency? (Such as bash pipelines)
- macrael 4y agoyet another reason I try and avoid throwing/rejecting promises in typescript code and just return `Thing | Error` everywhere. I'm sure there's something fancier I could get out of using a full Result type but this gets me a compiler enforcing I handle all errors somehow and keeps `try catch` out of my business logic.
- candiddevmike 4y agoHuh, never thought to do this. Coming from Go, this would honestly be very ergonomic (not being snarky).
- kansface 4y agoJS let’s you throw any type as an error.
- hot_gril 4y agoWell you know what they say, the better name for Go would've been Errlang.
- macrael 4y agoIt's honestly great. I also have done a fair amount of go (and before that a decent amount of objc) and it feels in the same vein as both of those but with some nice additional compiler helpers.
- rickstanley 4y agoI do it the "go" way, with: type Result<K = any, T extends Error = Error> = [K, null] | [null, T]; And use it: const [response, error] = await tryFetch<Type>(""); if (error) { handle(); }
- macrael 4y agoBut this way you are losing the type checker yelling at you if you try and use response without having handled error. What I like about res = something(): Thing | Error is I have to do if (res instanceof Error) { handle() return res } doSomethingWith(res) or else the compiler will yell at me for trying to doSomethingWith(Error)
- spankalee 4y agoI think the ideal thing to do in a lot of cases is to adhere to the spirit of the warning and actually try to handle the rejections in some way. In this case I'd want to log the error, or display a notice that the chapter couldn't be loaded. For this we need to catch the rejections and turn them into values we can iterate over with for await/of. Promise.allSettled() does this for us, but forces us to wait until all promises are... settled. A streaming version would give back an async iterable to we could handle promises in order as soon as they're ready. Or we can just convert into the same return type as allSettled() ourselves: async function showChapters(chapterURLs) { const chapterPromises = chapterURLs.map(async (url) => { try { const response = await fetch(url); return {status: "fulfilled", value: await response.json()}; } catch (e) { return {status: "rejected", reason: e}; } }); for await (const chapterData of chapterPromises) { // Make sure this function renders or logs something for rejections appendChapter(chapterData); } }
- horsawlarway 4y agoI'm laughing - I just posted basically the exact same thing. This is the right way to solve this. He's trying to shove error handling into the wrong place (The loop should not be responsible for handling failed requests it knows nothing about).
- ehnto 4y agoI don't think that is an absolute. Lets say it is paragraphs not chapters, the page needs to know to didn't get all the paragraphs to build the story so it can know the page has failed to complete. That's the perfect time to throw an exception and handle it somewhere that will know how to let the application continue to run sanely.
- horsawlarway 4y agoSure - I don't think anything is really an absolute in programming, and depending on the use case there are all sorts of ways to structure this. But in general, if you're making a contract where a promise might reject and using await, you need to be wrapping that code in a try/catch somewhere. That's the dealio with async/await and promises: you're trading the callback .then().catch() format for try{ await ... }catch(){}. If you don't want to use try/catch - you need to be making a contract where the failure is not a rejection but an error object. If you want to use rejections, you need to be catching them (in one form or another).
- horsawlarway 4y agoPersonally - he's trying to shove the logic to catch the error into the wrong place. The loop isn't the right spot to be catching an error that resulted from failing to fetch the data, or to parse the json data that was returned. The right spot was right there at the top of the file... const chapterPromises = chapterURLs.map(async (url) => { const response = await fetch(url); return response.json(); }); This is where the error is occuring, and this is also the only reasonable spot to handle it. A very simple change to const chapterPromises = chapterURLs.map(async (url) => { try { const response = await fetch(url); return response.json(); } catch(err) { // Optionally - Log this somewhere... return `Failed to fetch chapter data from ${url}` // Or whatever object appendChapter() expects with this as the content. } }); Solves the entire issue.
- jaffathecake 4y agoThat means you'll display chapter 3 even if chapter 2 fails, rather than explain that the story has failed to load. There's no point showing additional chapters if an earlier one fails.
- horsawlarway 4y agoNow you're pretty far out of a discussion about handling promise rejections, and into a discussion about product UX and feature design... "There's no point showing additional chapters if an earlier one fails." <- This is not always true, and heavily dependent on what the use case is. For a book? sure. for a manual or textbook? Much less sure. The approach works just fine either way, though - if you don't want to display additional chapters if one fails, you can do so easily by just including a "success" field (or anything similar) in the object that appendChapters expects, and break if needed when you hit that. (although again - you'd probably be better served by displaying a user readable error and offering a button to retry, and then displaying the rest of the content so you don't have to fetch it all a second time). Alternatively, you can also have appendChapters throw, or if you really want, you can still leave the promise rejection - but leaving the rejection or throwing at this spot means you're making a contract that calls for `try/catch` to use without unhandled rejections. That's the whole deal with async await. You're trading the formatting of .then().catch() for try{}catch(){}. if you don't like writing try/catch blocks, either structure the contract so that the promise doesn't reject (at least for expected error cases), or use the traditional .then().catch().
- latchkey 4y agoThis example is interesting because it is doing a fetch in a loop, which could fail for a whole number of reasons and leave you with limited data that you then need to build a UX to deal with ("Some data didn't load correctly, click here to try again"). The example is a fetch of each chapter in a book and this opens up a larger question of how to build APIs. If I was doing this, I'd have an API endpoint which returns all of the chapters data that I needed for the UX display in a single request. Then, I don't need the loop. The response is all or nothing and easier to surface in the UX. The next request would presumably just for an individual chapter for all the details of that chapter. Again, no loop. I know this doesn't answer the question of how to deal with the promises themselves, but more about how to design the system from the bottom up not not need unnecessarily complicated code.
- horsawlarway 4y agoEh, he's not really doing the fetch in a loop - he's generating a sequence of promises that are kicking off the fetch right away (during the call to map). If the number of chapters is less than the sliding window of outstanding requests in the user's browser - the browser will make all the requests basically in parallel. If the number of chapters is very large, they will be batched by the browser automatically - chromium used to be about 6 connections per host, max of 10 total, not sure if those are the current limits. He's just processing them in a loop as they resolve. The real issue with his code is that the loop isn't the right place to handle a rejection. The right place is right there at the top of the file in the map call. The loop doesn't need to care about a failed fetch or json parse, those should be handled in the map, where you can do something meaningful about it. The loop doesn't even have the chapterUrl of the failed call.
- spion 4y agoThis is why we need proper, rich stream abstractions in JS that integrate well with promises. Promises by themselves are not enough. Note that a real world implementation of this would also limit the number of concurrent requests - another reason to have streams with rich concurrency options.
- spankalee 4y agoModern JavaScript has fine stream abstractions in WHATWG Stream and async iterables, and Dart has the same unhandled Future issue as JS. Dart generally has fewer, but nicer, libraries for dealing with collections of Futures, but JS has a lot of npm libraries and is somewhat catching up with additions like Promise.{any,allSettled,race} and Array.fromAsync().
- spion 4y agoAsync iterables are bare-bones, and WHATWG Streams are not much better. I removed Dart from the comparison.
- no_wizard 4y agoThis is what observables are all about, yet `rxjs` and the like never gained any wide spread traction, despite solving so many of these issues very well. Not to mention there are many great upsides to using observables as well! I remember at the time Promise was gaining acceptance toward a standard there was a rich debate about adding observables and leveraging this as the async primitive but developers complained loudly that they wanted something more primitive. Then everyone complained about Promise being too verbose, so we got async await and async iterables. And now that we have said primitive, people say well where are the promise helpers, promises alone aren't enough! All of this could have been avoided with observables
- spion 4y agoObservables didn't properly integrate with promises at the right time - if they did they'd be accepted way more easily (e.g. `.first()` returning a Promise rather than an observable or `using` accepting promise return values and/or async functions and returning promises as a result). I gave the Dart API originally as an example, as it has careful considerations for when to return Future<T> instead of another Stream<T> (https://api.dart.dev/stable/2.18.7/dart-async/Stream-class.html https://api.dart.dev/stable/2.18.7/dart-async/Stream-class.h...) - examples include first(), last(), asyncMap etc.
- esprehn 4y agoI disagree that the fix is to catch and ignore the error. The fetch handling logic should be catching the errors and reporting them somewhere (ex. Sentry or the UI). In a production app where you report and log errors this "issue" doesn't manifest.
- akira2501 4y agoI usually catch rejections then turn them into a resolution as an object with an 'errno' property. That way, your handler always gets called, rejections are never generated up to the top level, and you can move the error handling into your consumer very easyily. const thing = await fn(); if (thing?.errno) { //handle error return; } // handle result Maybe I spent too much time writing C code. There are very few times when I actually want a rejection to take an entirely different path through my code, and this feels like the correct way to write it.
- jaffathecake 4y agoThe error isn't ignored. The parent function will still reject if it's unable to complete the operation (with the fetch error, if fetch was the cause of the error).
- esprehn 4y agoI didn't say it's globally ignored (I understand how promises work :)), I said what you did in the code sample adds a catch that ignores the error (it's an empty catch). In real code that wouldn't be empty though, it should have error reporting logic or something to handle that situation in the UI. Once you do that it's not a "hack" anymore, it's just reasonable error handling.
- jaffathecake 4y agoDo you object to Promise.all/race/any for turning potentially many rejections into one while marking all as handled?
- mirekrusin 4y agoSo you catch it and pass undefined to your chapter handler function? It will surely blow up. In production code I'm using `.catch(log.rescueError(msg, returnValue))` helper function (or .rescueWarn) - which is very helpful. It'll log error of that severity and return what I'm providing in case of exception.
- jaffathecake 4y ago> So you catch it and pass undefined to your chapter handler function? No, the original rejection is preserved.
- deleted 4y ago[deleted]
- neallindsay 4y agoI was strongly against JS engines treating unhandled rejections as errors because a rejected promise is only ever not handled yet. Basically, in order to prevent blow-ups from unhandled rejections, you have to synchronously decide how to handle rejections. I feel like this goes against the asynchronous nature of promises.
- londons_explore 4y agoWhat would the alternative be? Handle them when the last reference to the promise disappears? Seems that could hide more bugs when someone keeps a reference to a big set of promises for some other reason - keeping a reference to something shouldn't change that things behaviour.
- pohl 4y agoThis needs a trigger warning. I flashed back to upgrading a large project's build from node 12 to 16, which introduced a bunch of these in the scope of jest tests, and there was no indication of where the offending code was, which made the correct advice of "handle the rejections in some way" a little difficult.
- deleted 4y ago[deleted]
- difosfor 4y agoI was happy when Promise became available, but in retrospect I'd wish we would have skipped ahead and gotten Observable (e.g: https://rxjs.dev/ https://rxjs.dev/) instead to enable more powerful functionality and composition etc. In Typescript dealing with rejection is also painful since rejection reasons can't be guaranteed to be Error even when you always take care of that. And it can't help you guarantee that you're handling all types of errors thrown. For that purpose I'm thinking of using https://github.com/supermacro/neverthrow#readme https://github.com/supermacro/neverthrow#readme or https://swan-io.github.io/boxed https://swan-io.github.io/boxed.
- swyx 4y agoboth can coexist, observable is multiple-push while promise is single-push. they have their place.
- clarkdale 4y agoThe change in mine is moving await into the second for loop, so that it can be caught and handled. async function showChapters(chapterURLs) { const chapterPromises = chapterURLs.map(async (url) => { const response = await fetch(url); return response.json(); }); for (const i in chapterPromises) { try { appendChapter(await chapterPromises[i])); } catch (error) { handleChapterErrorGracefully(error, i); } } } I used for..in so that the index can be passed to handleChapterErrorGracefully
- clarkdale 4y agoHere is a way with a Generator (the chapterGenerator). Only writing this because I've seen a lot of comments lamenting lack of "streams" async function showChapters(chapterURLs) { const chapterGenerator = function*() { for(const url of chapterURLs) { yield fetch(url).then(response => response.json()); } }; for (const chapter of chapterGenerator()) { try { appendChapter(await chapter)); } catch (error) { handleChapterErrorGracefully(error); } } }
- DecoPerson 4y agoI don’t believe this will resolve the issue. - Promises are created: 1,2,3,4 - The for loop awaits promise 1 - Promise 3 is rejected - Promise 1 is resolved - The for loop awaits promise 2 - … Somewhere in this, promise 3’s rejection would be considered unhandled (I’m not sure exactly when).
- clarkdale 4y agoPromise 3's rejection would be awaited and handled after promise 2. Yes, it might finish over the network before promise 1, but it won't be realized by the program until its await occurs in the for loop.
- DecoPerson 4y agoThe issue is that V8 unhandled rejection logic doesn’t wait until the promise is awaited. If a promise is rejected, has no awaiter (.then/.catch/await), and persists in this state for some amount of time/event-loops (that the article covers), it will be treated as unhandled. I tested this in Node 18. It treats it as unhandled. The only way to avoid the gotcha’s described in the article is to ensure all promises: - cannot fail, and/or - are immediately awaited (by anything but .catch is the easiest).
- deleted 4y ago[deleted]
- deleted 4y ago[deleted]
- deleted 4y ago[deleted]
- ehnto 4y agoOne day I will write an article titled "I wish you had just built this syncronously, I can wait." Speaking in the context of apps and webapps, so many bad UX moments are caused by async optimizations that are entirely pointless. Often I can't action the page until all the calls are done anyway, I would much rather see a finished page than watch the jittery pained birth of a dozen async calls coming to fruition and not knowing when it's safe to click something.
- CGamesPlay 4y agoI just discovered that this is possible to a limited degree using the old XMLHTTPRequest class. Looks like it has some limitations though. https://www.npmjs.com/package/sync-fetch https://www.npmjs.com/package/sync-fetch
- rolldog 4y agoGmail async loading attachments and moving everything down a row when they do has been killing me in the web app recently. Hate it. Have to consciously wait a few seconds before use every time now.
- horsawlarway 4y agoNah - you really don't want to make it sync. Personally - I don't want my entire UI to lock up on slow connections because the entire js context is paused waiting for a response that was fast for the dev on his dev machine sitting right next to the server, but is slow as fuck on my 3g connection pulling data at 1kb a second with fairly frequent packet loss. I'd like to be able to click other links, navigate through menus, and interact with the site if I'm exploring and just happened to click on this page that's now loading a whole book.
- Too 4y agoPythons asyncio.as_completed has a neat solution to this. By returning a placeholder-promise (awaitable), instead of the value they resolve to. It is only when you await this placeholder it eventually resolves the value of the first completed item in the loop. With this design you can choose yourself what do do if it fails. for coro in asyncio.as_completed(awaitable_list): earliest_result = await coro # Can be wrapped with try-block I think same thing can be achieved with smart use of Promise.race. It's a bit different from the article, that wants to iterate in same order as the input. This can be added, by collecting the results and iterating them again, reinventing Promise.allSettled, i didn't fully understand why op couldn't use that.
- flippinburgers 4y agoAnd this is why we shouldn't allow frontend engineers to get ahead of themselves.