No panic on fetch error by avsaase · Pul...

I'm curious to hear what you guys think of this? https://github.com/cloudflare/workers-rs/pull/524

GitHub

No panic on fetch error by avsaase · Pull Request #524 · cloudflare...

This PR returns a generic 500 response to the client, instead of panicking, when an error is returned to the fetch handler. IMO panicking defeats the purpose of Rust's error handling model and ...

46 Replies

dakom•9mo ago

my opinion (ymmv): I like to reserve panics for things that really truly should panic, and be able to tell the difference in the logs. I think forcing to never panic is a bit too opinionated, but I haven't deployed any workers to production or done live debugging / put out fires... so take with a grain of salt. as an example - if I'm doing a bunch of math stuff and I'm really really sure that I should never have a divide by zero, I want that to panic - it's more about catching developer bugs than runtime errors like "user not found"

avsaaseOP•9mo ago

You can still panic in your own code. This change just makes it so that the worker crate doesn't panic for you when you do this

#[event(fetch)]
async fn fetch(
    _req: HttpRequest,
    _env: Env,
    _ctx: Context,
) -> worker::Result<http::Response<axum::body::Body>> {
    Err(worker::Error::RustError("Error".to_string()))
}

#[event(fetch)]
async fn fetch(
    _req: HttpRequest,
    _env: Env,
    _ctx: Context,
) -> worker::Result<http::Response<axum::body::Body>> {
    Err(worker::Error::RustError("Error".to_string()))
}

dakom•9mo ago

oh, I misunderstood... in that case, yeah, I agree 👍 hehe, in other words:

I think forcing to never panic is a bit too opinionated

it's really:

I agree forcing to always panic is a bit too opinionated

😉 also curious to hear the use-case where it makes sense to always panic

Unknown User•9mo ago

Message Not Public

avsaaseOP•9mo ago

Yeah. And I don't like that it's hidden like this

Unknown User•9mo ago

Message Not Public

avsaaseOP•9mo ago

My whole error handling setup is basically useless this way. I might as well call unwrap myself on every result or option.

Enduriel•9mo ago

I personally dislike the fact that we return a result anyway tbh, I've added intermediary functions that add Ok() around the actual functions I want to route to because I don't want to forget to hand an error state somewhere. Am I just strange in that way?

avsaaseOP•9mo ago

No I think you're right. I guess the idea was to facilitate error handling using the the question mark operator but in practice you still need to do your own mapping. It's also inconsistent because the scheduled event macro does not accept a function that returns a result. In general I think the error handling should be reconsidered.

Unknown User•9mo ago

Message Not Public

avsaaseOP•9mo ago

I prefer defining my own error types with thiserror. anyhow makes it easy to propagate the error but it makes it harder to differentiate the errors later on.

Unknown User•9mo ago

Message Not Public

avsaaseOP•9mo ago

FYI, the worker::Error type does not implement the Error trait So you can't use ? on a worker error to propagate it to an anyhow::Result

Unknown User•9mo ago

Message Not Public

avsaaseOP•9mo ago

Idk, anyhow is somewhat opinionated Intuitively it makes sense to me that the worker crate gives me worker::Errors so I need to give that same type back.

Unknown User•9mo ago

Message Not Public

avsaaseOP•9mo ago

That's a good idea

dakom•9mo ago

So, one of the main benefits I like about this projects is that I can share types with the frontend- and that includes errors 🙂 Fwiw what I am doing atm is using thiserror and a custom error type that impls From<worker::Error> so ? works most of the time, and map_err(|err| err.into()) when it doesn't main is then a thin wrapper which converts MyError to worker::Error right before returning Clientside it just works, parsing back from the response string via serde magic I don't think this conflicts with any of the above ideas though, just sharing in case it's helpful I can't remember why I didn't use anyhow everywhere for that flow.. given that it's application side, should probably try that again 😅

avsaaseOP•9mo ago

The advice to "use anyhow for applications" is pretty bad imo. Not saying that you're saying that but using anyhow for all your errors generally doesn't encourage good error handling strategies

dakom•9mo ago

Interesting.. not sure I'm not saying that, hehe. Though I know I have been bitten by trying to use it in libraries, so, I guess it's applications by elimination How has it led to bad error handling for you? In my totally subjective anecdotal experience, I usually only need to do one of these: 1) log the error (anyhow is fine) 2) send the error over the wire (type gets lost anyway, recovered via serde on the other side) 3) get the specific error for limited niche test code cases (anyhow downcast is a bit annoying but fine) Maybe I'm just not doing as much error-specific testing? In general I haven't done a lot of serverside or crucial cli tools, my Rust experience is very biased by frontend and Blockchain smart contracts, both of which have their pretty weird things (in frontend everything crosses the FFI and has to be static, in Blockchain there's basically no async) Pretty much all the backend/cli tools I've done in Rust were either led by a different developer or not crucial to do nicely, so it's likely I haven't thought hard enough about good error handling in servers Happy to learn more!

avsaaseOP•9mo ago

I would summarize it like this: - Need to know that an error occurred but it doesn't matter which one? -> anyhow is great - Only need to textual representation of the error? (this usually only applies in CLI tools) -> anyhow is great - Need custom handling of different error types/variants? -> thiserror.

Unknown User•9mo ago

Message Not Public

avsaaseOP•9mo ago

Left a comment

Unknown User•9mo ago

Message Not Public

avsaaseOP•9mo ago

Oh it does?

Unknown User•9mo ago

Message Not Public

avsaaseOP•9mo ago

Didn't use to be the case iirc

Unknown User•9mo ago

Message Not Public

avsaaseOP•9mo ago

Git blame says 3 years ago 🤔 Maybe I'm confusing it with another error type

Unknown User•9mo ago

Message Not Public

avsaaseOP•9mo ago

I'm afk now. I'll have a look later.

Unknown User•9mo ago

Message Not Public

avsaaseOP•9mo ago

Maybe something like this to check if the tour implements Error? https://github.com/nvzqz/impls

GitHub

GitHub - nvzqz/impls: A Rust macro to determine if a type implement...

A Rust macro to determine if a type implements a logical trait expression - nvzqz/impls

avsaaseOP•9mo ago

Probably overkill

Unknown User•9mo ago

Message Not Public

avsaaseOP•9mo ago

Yeah, the link "to_string not found" -> missing Display impl -> missing Error impl is a bit opaque. I guess the last step is not strictly necessary since we're only stringyfying the error.

Enduriel•9mo ago

@kflansburg how would you feel about changing the signature of the main fetch function from Result<Response> to Response? that would be an alternative solution to this issue which imo makes more sense hollistically.

Unknown User•9mo ago

Message Not Public

Enduriel•9mo ago

You think that's a good pattern? Or do you just think it makes it more accessible and therefore it should be kept that way to increase accessibility? This I think is useful regardless

Unknown User•9mo ago

Message Not Public

Enduriel•9mo ago

I see, fair enough

dakom•9mo ago

Someone suggested implementing IntoResponse for worker::Error which I think makes sense

that's similar to what I'm doing, but with my thiserror type (which has a From<worker::Error> impl)

avsaaseOP•9mo ago

I think axum specific stuff should be behind an axum feature flag.

Unknown User•9mo ago

Message Not Public

avsaaseOP•9mo ago

That would also open up the possibility for more tight Axum integration.

Unknown User•9mo ago

Message Not Public

Gaming

Programming

No panic on fetch error by avsaase · Pul...