Async Request Handlers by customizing error response #529

darkdarkfruit · 2021-03-04T08:37:08Z

/// # customize response for HandlerError
/// ## Why do we need it?
/// We might want to customize different response for different error, eg:
/// * for authorized-user-resource, we might return 403(Forbidden) for an un-logged-in user;
/// * or when some file user requesting is not found, we might return 404;
/// * ...
/// Or we just want to send with content-type: application/json when request is application/json.
/// (In the old, we just send 500 with plain/text for any request if error happens)

codecov · 2021-03-04T08:55:47Z

Codecov Report

Merging #529 (4d269e2) into master (4bd963a) will decrease coverage by 0.13%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##           master     #529      +/-   ##
==========================================
- Coverage   84.99%   84.85%   -0.14%     
==========================================
  Files         110      111       +1     
  Lines        5617     5757     +140     
==========================================
+ Hits         4774     4885     +111     
- Misses        843      872      +29

Impacted Files	Coverage Δ
gotham/src/handler/mod.rs	`79.16% <ø> (ø)`
gotham/src/handler/error.rs	`67.10% <70.00%> (+1.79%)`	⬆️
...s_await_with_customized_error_response/src/main.rs	`78.76% <78.76%> (ø)`
gotham/src/middleware/session/backend/memory.rs	`90.90% <0.00%> (+1.01%)`	⬆️
gotham/src/router/builder/single.rs	`97.05% <0.00%> (+5.88%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4bd963a...76e6464. Read the comment docs.

msrd0 · 2021-03-04T13:10:24Z

I don't understand why this is neccessary. Looking at your example code

    let _io_error = Err(std::io::Error::last_os_error())
        .map_err_with_customized_response(
            state,
            |_state| {
                // an error occurs, but still sending **OK** to client
                (StatusCode::OK, mime::TEXT_PLAIN_UTF_8, "Customized response by the last os error (Intentionally return 200 even error occurs)")
            },
        )?;

Why don't you just write your code like this:

    let io_result = Err(std::io::Error::last_os_error());
    let _result = match io_result {
        Ok(result) =>  result,
        Err(_) => {
            let resp = create_response(&state, StatusCode::OK, mime::TEXT_PLAIN_UTF_8, "Custom error response");
            return (state, resp);
        }
    }
    // do whatever with result to create your non-error response

darkdarkfruit · 2021-03-04T22:37:36Z

Well we can use the '?' shorthand.

msrd0 · 2021-03-04T23:08:31Z

I see what you are trying to acchieve but I don't think this is the way to go. In all of your examples, you did not specify a concrete type for Result<T, std::io::Error>. For the first three examples, I'm actually suprised they compile and the compiler doesn't require you to choose a concrete type. However, the type is never accessed, so you'd probably get away with (). You can then create an empty response since the state is only borrowed, eventhough that return is unreachable (but the compiler doesn't know).

The 4th example (that uses to_async) works because gotham expect the concrete type (State, Response<Body>), but the Ok variant of that result has no chance of ever being created. Not only because you've hardcoded it, but also because you are moving the state in .map_err(|e| (state, e)) so you can never return a non-error response from that handler. This is my main problem with your Pull Request.

If you believe I'm wrong, please add a test/example for both to_async_borrowing and to_async where the handler has a chance of returning both an error and a non-error response (for example by "seeding" the result from some other fn maybe_error() -> Result<String, Error> { Err(Error::last_os_error()) }).

darkdarkfruit · 2021-03-04T23:54:37Z

For "to_async_borrowing", it is easy to return maybe_error;
But for "to_async", it can't(Because according to handler trait, the state must be moved in and state does not implement "Clone"), and that is not the right place to use it. The "to_async" example might be deleted from confusing the user.

msrd0 · 2021-03-04T23:57:58Z

Yes, that is exactly what I said. We're supporting both handler styles, owned and borrowed, and HandlerError is used for both of those as well. You will need to provide a way to make it work with both to_async and to_async_borrowing, which I don't think is possible with your approach.

darkdarkfruit · 2021-03-05T00:53:06Z

So is there any way to use '?' shorthand in both cases? By handler design, it seems impossible, can you/someone shed some ideas?

msrd0 · 2021-03-05T01:00:59Z

No, I'm not talking about ?, that will only work with borrowing. I mean that HandlerError is used with both borrowing and owned state handlers, so if you add additional information to it (like a custom response), it should be usable from both handler types. Eventhough I still don't fully like that solution as it makes the return type of every handler take about twice as much memory as before, but that could be fixed by using Option<Box<Response<Body>>> inside the HandlerError.

msrd0 · 2021-03-05T01:02:18Z

The best solution to this would be using Try but unfortunately that is not available in stable Rust so we can't currently use it.

darkdarkfruit · 2021-03-05T02:34:51Z

Just for curious, Option<Box<Response<Body>>> takes a box pointer which still points to the heap, why is it better than Option<Response<Body>> at memory saving?

msrd0 · 2021-03-05T02:43:45Z

Well, I'm not an expert in this field and I also have no benchmarks to backup any assumptions. But Option<Response<Body>> is always one byte longer than the size of Response<Body> whereas Option<Box<T>> is always just the size of a pointer unless used. Ideally, we'd only require the space for one response, no matter if it is an Ok or Err result.

Btw is there any reason why you prefer using ? over a match block? Looking at the example I gave earlier, both require about the same lines of code.

darkdarkfruit · 2021-03-05T03:03:19Z

Thanks very much, your explanation makes sense.

Why I prefer ? ?
One of my real world business code have to deal with many possible errors and if I can chain them with '?', the logic is more intuitive.

msrd0 · 2021-03-05T13:09:13Z

Maybe you are mixing your own logic with gotham's logic. Gotham (as of right now) expects the handler to return an error only if it is unable to produce a response, no matter if that is response has a 200, 400 or 500 status code, and that kind of makes sense to me.

For your use case, I'd suggest you create some custom error type like

#[derive(Debug, Error)]
enum MyError {
    #[error("I/O Error: {0}")]
    IoError(#[from] std::io::Error),
    #[error("{0}")]
    Other(String)
}

impl MyError {
    fn try_into_response() -> HandlerResult { /* create custom error response */ }
}

and then have your handler return some Result<Response<Body>, MyError> and call your handler like this:

route.get("/foo").to(|state| {
    let res = my_handler(&state).map(|resp| Ok(resp)).unwrap_or_else(|err| err.try_into_response());
    match res {
        Ok(resp) => Ok((state, resp)),
        Err(err) => Err((state, err))
    }
});

This way you can use the ? shorthand in the my_handler function. You could also move the code of above closure into some wrapper method, so you'll only have to call .to(|state| wrap_handler(state, my_handler)). Does that work for you?

darkdarkfruit · 2021-03-05T22:29:45Z

Thanks for the very useful tip.
It helps but could not replace map_err_... in handler, here are my some thoughts:

There are many errors tighting too close to the context inside and they are hard to extract to a single place.
Some repeated code on route(macro might help), and we might easily forget them.
Reduces ergonomics a little bit.

msrd0 · 2021-03-08T14:01:41Z

Thanks for your thoughts, but I still think that a response with a status code that indicates an error to the http client is still a successful response from gotham's perspective as the handler was able to produce a response.

darkdarkfruit · 2021-03-10T08:48:44Z

OK, thanks.

Async Request Handlers by customizing error response

5f0312d

darkdarkfruit added 3 commits March 6, 2021 05:50

use Option<Box<Response<Body>>> instead of Option<Response<Body>>

0470325

Add "Additional feature of this fork" to project README.md

cafd965

rustfmt

76e6464

msrd0 closed this Mar 8, 2021

msrd0 mentioned this pull request May 2, 2021

HandlerError with body data #542

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async Request Handlers by customizing error response #529

Async Request Handlers by customizing error response #529

darkdarkfruit commented Mar 4, 2021

codecov bot commented Mar 4, 2021 •

edited

Loading

msrd0 commented Mar 4, 2021

darkdarkfruit commented Mar 4, 2021

msrd0 commented Mar 4, 2021

darkdarkfruit commented Mar 4, 2021

msrd0 commented Mar 4, 2021

darkdarkfruit commented Mar 5, 2021

msrd0 commented Mar 5, 2021

msrd0 commented Mar 5, 2021

darkdarkfruit commented Mar 5, 2021

msrd0 commented Mar 5, 2021

darkdarkfruit commented Mar 5, 2021

msrd0 commented Mar 5, 2021

darkdarkfruit commented Mar 5, 2021

msrd0 commented Mar 8, 2021

darkdarkfruit commented Mar 10, 2021

Async Request Handlers by customizing error response #529

Async Request Handlers by customizing error response #529

Conversation

darkdarkfruit commented Mar 4, 2021

codecov bot commented Mar 4, 2021 • edited Loading

Codecov Report

msrd0 commented Mar 4, 2021

darkdarkfruit commented Mar 4, 2021

msrd0 commented Mar 4, 2021

darkdarkfruit commented Mar 4, 2021

msrd0 commented Mar 4, 2021

darkdarkfruit commented Mar 5, 2021

msrd0 commented Mar 5, 2021

msrd0 commented Mar 5, 2021

darkdarkfruit commented Mar 5, 2021

msrd0 commented Mar 5, 2021

darkdarkfruit commented Mar 5, 2021

msrd0 commented Mar 5, 2021

darkdarkfruit commented Mar 5, 2021

msrd0 commented Mar 8, 2021

darkdarkfruit commented Mar 10, 2021

codecov bot commented Mar 4, 2021 •

edited

Loading