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

How should Body handle errors? #1951

Closed
davidpdrsn opened this issue Apr 21, 2023 · 9 comments
Closed

How should Body handle errors? #1951

davidpdrsn opened this issue Apr 21, 2023 · 9 comments
Labels
A-axum-core I-needs-decision Issues in need of decision.
Milestone

Comments

@davidpdrsn
Copy link
Member

In 0.7 axum is getting its own Body type. Currently hyper just ignores errors produced by bodies which is a bit of a footgun. We should consider whether axum's body should use Infallible as the error type.

@davidpdrsn davidpdrsn added this to the 0.7 milestone Apr 21, 2023
@davidpdrsn davidpdrsn added I-needs-decision Issues in need of decision. A-axum-core labels Apr 21, 2023
@rakshith-ravi
Copy link
Contributor

If not for Infallible, what's our other option? Also, what're the reasons a body can fail? Are we talking about network failures when sending data or something else?

@seanmonstar
Copy link
Member

hyper doesn't ignore errors, an error from a body means the connection gets closed abruptly. Transmission and generations of bodies can fail part-way through, so Infallible doesn't seem correct.

@rakshith-ravi
Copy link
Contributor

rakshith-ravi commented Sep 29, 2023

In that case I think the only time when we should actually handle errors for Body is for upgraded requests (like websockets). If a JSON body is being sent, and the connection gets closed abruptly, there's not much we (or the user) can do anyway. We can just ignore that part and not expose that to the user.

For upgraded requests where connections are kept open though, it certainly makes sense to handle errors for Body and provide the user with a way to handle it

@davidpdrsn
Copy link
Member Author

The primary use case I have in mind is making it easy to log the error or send it to whatever error reporting system a user wants to use.

@rakshith-ravi
Copy link
Contributor

Maybe we can just do a simple log::info!() with a specific target and let the user route that target to wherever they want?

@davidpdrsn davidpdrsn modified the milestones: 0.7, 0.8 Nov 10, 2023
@happysalada
Copy link

Indeed, i just shot my foot :-)

In the serve with hyper example, on an error, no 500 responses are returned, it just closes the connection abruptly. Im happy to contribute a PR to "fix" the example if there anyone has pointers.

My question becomes, how does one take those errors and make hyper return a 500 from it ?

@jplatte
Copy link
Member

jplatte commented Sep 24, 2024

@happysalada that's impossible, at the time the body error is generated, the status code has already been encoded and possibly sent.

@happysalada
Copy link

This makes sense.

I seem to remember that an error in the stream caused a panic and the server to stop accepting request.

It seems in this case, closing the connection abruptly makes sense, it should just fail gracefully and leave am option to log the error somewhere

@jplatte
Copy link
Member

jplatte commented Sep 28, 2024

I'm not really seeing anything actionable here, as noted by Sean, Infallible isn't really adequate.

@jplatte jplatte closed this as not planned Won't fix, can't repro, duplicate, stale Sep 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-axum-core I-needs-decision Issues in need of decision.
Projects
None yet
Development

No branches or pull requests

5 participants