[HN Gopher] The gotcha of unhandled promise rejections
___________________________________________________________________
The gotcha of unhandled promise rejections
Author : AshleysBrain
Score : 65 points
Date : 2023-01-12 00:12 UTC (1 days ago)
(HTM) web link (jakearchibald.com)
(TXT) w3m dump (jakearchibald.com)
| mirekrusin wrote:
| So 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.
| spion wrote:
| This 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 wrote:
| Modern 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 wrote:
| Async iterables are bare-bones, and WHATWG Streams are not
| much better.
|
| I removed Dart from the comparison.
| spankalee wrote:
| I 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); } }
| [deleted]
| spion wrote:
| Nice. `Promise.iterateSettled` would be cool.
| horsawlarway wrote:
| I'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).
| crdrost wrote:
| This code does not recapitulate the desired behavior.
|
| Desired: when the load from chapter 10 fails, even if chapter 2
| is taking 30s to load, I want to immediately transition the UI
| into the failure state.
|
| Your code: waits until chapters 1-9 have loaded to report the
| failure state on chapter 10.
|
| (I posted I believe a correct answer "without the hack" to the
| OP.)
| spankalee wrote:
| I'm not trying to say there's only one way to do this,
| especially if we are talking about user affordances for
| failure states. I think there could be a debate about showing
| what data is available vs not showing partial data.
|
| My point is just that trying to do something with the
| failures often removes the warning and leads to better UX, or
| at least conscious decisions about it, and usually some clue
| in the code about what you intend to happen.
| esprehn wrote:
| I also posted the same thing (and have had this discussion with
| Jake on Twitter :))
| [deleted]
| [deleted]
| horsawlarway wrote:
| Personally - 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.
| latchkey wrote:
| This 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 wrote:
| Eh, 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.
| latchkey wrote:
| Each user is making 6-10 requests, in parallel, to the
| server, instead of 1. My point is that I'd design this to not
| DDoS the servers if I got a lot of concurrent users.
| horsawlarway wrote:
| that's... not a particularly astute approach.
|
| Generally speaking, you're almost always better off with
| more light requests than fewer heavy requests.
|
| Assuming this is static content (chapters) there's
| absolutely zilch you're gaining by fetching the whole block
| at once instead of chapter by chapter, since you can shove
| it all behind a CDN anyways.
|
| You're losing a lot of flexibility and responsiveness on
| slow connections, though.
|
| Not to mention - it may make sense to be doing something
| like fetching say... 5 chapters at a time, and then
| fetching the next chapter when the user scrolls to the end
| of the current chapter (pagination - a sane approach used
| by anyone who's had to deal with data at scale)
|
| In basically every case though, as long as each chapter is
| at least a few hundred characters, the extra overhead from
| the requests is basically negligible, and if they're static
| - easily cached.
| spankalee wrote:
| Most likely Jake encountered real world cases that really
| needed to use parallel promises and used fetch() as an
| example that could fit in a blog.
|
| Maybe in the real world it's file or database reads, off-
| thread parsing, decompression, etc...
| latchkey wrote:
| Yes, this is why I wrote:
|
| > _I know this doesn 't answer the question of how to
| deal with the promises themselves_
|
| I'm trying to bring up a bigger picture here.
| pyrolistical wrote:
| This 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
| neallindsay wrote:
| I 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.
| pohl wrote:
| This 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.
| samsquire wrote:
| It'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
|
| 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-mult...
| tech2 wrote:
| Python 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....
| macrael wrote:
| yet 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 wrote:
| Huh, never thought to do this. Coming from Go, this would
| honestly be very ergonomic (not being snarky).
| hot_gril wrote:
| Well you know what they say, the better name for Go would've
| been Errlang.
| kansface wrote:
| JS let's you throw any type as an error.
| tluyben2 wrote:
| This is what we do; for me it came from ancient times when I
| was doing fulltime C dev and I mostly kept it up (but improved)
| except when working on teams that want or already have
| exceptions all over in all layers.
| hot_gril wrote:
| That's ok, and I get the purity of only using the response
| instead of an implicit exception, but it gets tedious when
| probably almost every func can return an error. You only have
| to handle exceptions at the outermost layer. I don't understand
| why this is seen as a "gotcha" in the article, it's the whole
| point of exceptions.
| rickstanley wrote:
| I 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 wrote:
| But 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)
| esprehn wrote:
| I 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 wrote:
| I 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.
___________________________________________________________________
(page generated 2023-01-13 23:00 UTC)