9 ms·

The problem with Go’s default HTTP handlers

192 points by p5v 4y ago

topicseed 4y ago
I agree, I wouldn't have been against a required return, but the mitigation is relatively straightforward by using custom handlers like mentioned at the end, if needed. I really like Go's default HTTP handlers because it offers that straightforward flexibility. Nothing rocket-sciencey.
- antihero 4y ago
  Well yeah, I think it’s important to let you write bytes to the socket and close it when you want to, and if people are paranoid about making obvious, easy to spot bugs, they can use some abstraction to protect themselves.
- masklinn 4y ago
  > the mitigation is relatively straightforward by using custom handlers like mentioned at the end, if needed. Why not implement ServeHTTP on the custom handler though? That's not exactly difficult: type MyHandlerFunc func(w http.ResponseWriter, r *http.Request) error func (f MyHandlerFunc) ServeHTTP(w http.ResponseWriter, r *http.Request) { err := f(w, r) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) } } Then you can just register it with http.Handle("/hello", MyHandlerFunc(someHandler))
  - andrewstuart2 4y ago
    There are definitely edge cases here (like what if the handler already wrote a body and it was sent on the wire, etc), but this is absolutely the pattern to use, and one of my favorite parts of a type system that allows methods on function types.
    - catlifeonmars 4y ago
      Yes, or what if you want to upgrade the connection to a websocket?
  - skrtskrt 4y ago
    Actually a bunch of router/web helper libraries do this in different ways which makes it a huge pain to compose middleware from different libraries. Even Mux's middleware library is not consistent in how it does this for different middlewares so they have to be applied different ways or even have wrapper functions written
    - catlifeonmars 4y ago
      This. Middlewares typically expect to wrap an http.Handler so you lose all the middleware composition ergonomics by adding a return value to your custom handler signature
    - masklinn 4y ago
      Here it just implements the standard library’s interface rather than use some sort of conversion closure adapter, there’s nothing special to it.
- andrewstuart2 4y ago
  The beauty is in the simplicity. `http.Handler` is all about what is needed to handle HTTP and nothing else. It doesn't intend to make things pretty, or safe, or anything really, except to express the minimum surface area required to interact with HTTP while abstracting the commonalities. Beyond that, as the article mentions, it's quite easy to abstract and implement your own sugar layer.
mrweasel 4y ago
It's not something I even thought about, but I guess I see the point, I just don't agree. In the HTTP handlers it makes sense that you don't have return values, because: What would you do with that value exactly? The HTTP handler should always ensure that "something" is returned, at the very least a return code. Once something has been returned to the client, can you really argue that there's an error? There's a point to the risk of missing return value, you just run the risk of complicating the request flow by adding it. I'd argue that most people would view the code path as having ended, once the client receives a response. The request shouldn't really continue on the server end after that. There can be background processing, go routines or something like that, but that's a bit different, as it doesn't involve the original requests-response.
- jerf 4y ago
  "In the HTTP handlers it makes sense that you don't have return values, because: What would you do with that value exactly?" I agree that the baseline net/http implementation is correct. There is no appropriate default error (return) handler that the framework could implement, at the level it lives at, that would be correct and wouldn't be limiting. However I very often in my own Go code immediately create an abstraction that does exactly this, because it is a good default for everything I'm doing. It just isn't suitable for everything (e.g., what do you do with this in the case of a protocol upgrade to websockets? the HTTP handlers need to be able to just terminate gracefully without being forced to "return" something). What I would say is, net/http is a little too low level to directly use. You don't need a full framework necessarily, though if you want to use one that does this more power to you. But if you understand HTTP, it doesn't take much wrapping to make net/http useful for "normal" APIs or websites. It takes a little, though.
  - aniforprez 4y ago
    "net/http" doesn't seem to come with a lot of great defaults that almost every web app would use such as named path parameters which you'd need to write a bunch of code and regex to extract, middleware, routing based on HTTP method etc. This is all stuff you'd have to implement anyway I'd much prefer using something that's an abstraction of it like Gin, Chi, Echo and the like. They're fairly performative and add very little overhead on top of the standard package
dralley 4y ago
I like the way Axum for Rust handles this. All response handlers return something that implements IntoResponse. Lots of types implement IntoResponse, thus making the simple cases really streamlined. async fn create_user( Json(payload): Json<CreateUser>, ) -> impl IntoResponse { let user = User { id: 1337, username: payload.username, }; // this will be converted into a JSON response // with a status code of `201 Created` (StatusCode::CREATED, Json(user)) } If the handler is complex enough that branches can't all return the same type, you can just add .into_response() to each return site, and now you're just returning a response, which also implements IntoResponse (as a no-op). This avoids the problem discussed in this article while also avoiding much of the boilerplate of manually building responses.
- masklinn 4y ago
  I’ve not yet gotten to use it, but I understand Axum also implements IntoResponse for Result<A: IntoResponse, B: IntoResponse>, which is very convenient for the sort of error code of the above. It’s one of my biggest annoyance with Warp: it only implements Reply for `Result<T, http::error::Error>`, so if you have a faillible handler with more complicated error types (e.g. with a json body, or even a 200 response for RPC handlers) you’re in the awkward position of having to join the paths back (usually by converting everything to a Response by hand), which makes mistakes easier.
  - zaarn 4y ago
    Correct, I spent about half an hour implementing IntoResponse for my own project's error type, but now I can just use normal error handling and I get presented with an error page on any issues. It's quite magic (not really, very streamlined).
- pram 4y ago
  That is what Gin does.
  - maccard 4y ago
    Gin is excellent. It's the right balance of batteries included, sensible defaults, and black magic
halfmatthalfcat 4y ago
This isn't so much a critique of the HTTP handlers themselves but relying on (and stuffing a ton of shit into) context as a way of dealing with complex request/response patterns was a major headache. Trying to implement a circuit breaker against multiple upstreams for a single request in Go was a nightmare.
- morelisp 4y ago
  People put way too much stuff in the context (whether that's the req.Context() or the gin.Context or whatever). It should only be for things that are request-scoped and need to cross middlewares that aren't aware of them. Most such middlewares (logging, metrics, last-ditch error handling) get put at the front of the chain anyway. A circuit-breaker is basically by definition not request-scoped, so should not go in the context.
boredumb 4y ago
func (api \*App) HandleOnError(w http.ResponseWriter, err error, status int, context string) bool { if err != nil { api.logging.Error(err, context) http.Error(w, err.Error(), http.StatusInternalServerError) return true } return false } and then it's used in the handler funcs if api.HandleOnError(w, err, http.StatusInternalServerError, "Getting stuff for things") { return } Makes the logging verbose and handling always properly taken care of, makes the error handling in handlers verbose enough from the caller but abstract enough to not be a majority of the handlers body.
ainar-g 4y ago
Another possible extension of that design is using error types for HTTP codes. Something like: type HTTPError struct { Err error Code int } And then, with a couple of wrappers like this: func errorNotFound(err error) (wrapped error) { return &HTTPError{ Err: err, Code: http.StatusNotFound, } } you could do something like this: return errorNotFound(err) or this: return errorInternal(err) // 500 Internal Server Error
- _adamb 4y ago
  I usually create an abstraction like this in an API to require all returned errors to come with some sort of machine readable code. You either have to classify this response with an existing code in the list or add a new one. That list then gets generated as part of the documentation so consumers are getting possible error states straight from the code (without having to look at said code).
- marcus_holmes 4y ago
  I do similar: I create a `handleError` function that deals with the error (logs it, reports it, redirects to the appropriate page), and call that in a return statement. It's like 5 minute's work to code up, and I get to set whatever logging destination and redirect logic works best for my project.
JustSomeNobody 4y ago
They're not functions, they're procedures. Procedures don't return values. Write your code accordingly.
badhombres 4y ago
To me this reads as a positive to the default handlers, as it shows how easy it was to make it work into a format that works for the programmer's preference. I appreciated the solution to the complaint
deleted 4y ago
[deleted]
alectic 4y ago
Streamed-response use cases get weird/complicated if you have to return the response.
- masklinn 4y ago
  Sure but is that the usual / default? Why not have a second adapter?
- deathanatos 4y ago
  Not necessarily? The type used for Response can include a "body" type that supports streaming. E.g., body: AsyncRead in Rust. In Go I think it would resemble a channel that emitted the contents of the body. (As a library, though, you'd also want to make the simple non-streaming case of "just return this blob of bytes" simple for the consumer, too.)
alingse 4y ago
we have already do this in prod. type HandlerFuncErr func(w http.ResponseWriter, r http.Request) error func HandleErr(h HandlerFuncErr) http.HandlerFunc { return func(w http.ResponseWriter, r http.Request) { ... } }
chrsig 4y ago
I dont necessarily agree they should return a value, but I do have some issues with the api: - by default, you can't tell if a handler has already written to the body or emitted a status code. - by default, you can't read the body more than once - r.PostForm vs r.Form ...that they both exist, and that they're only available after a manual call to r.ParseForm - the http server accepts an unbounded number of connections. it'll just keep making goroutines as they come in. there's no way that I know of to apply back pressure. there's probably more that I grump about, but those are the ones at the top of my mind. for the most part it works well, and i can work around my gripes.
- morelisp 4y ago
  - by default, you can't read the body more than once This is a reasonable default - probably the only reasonable default. `TeeReader` and `MultiReader` are easily available if you want to spare the memory. (But you're right that the converse on the ResponseWriter isn't true, it's much more difficult to get your own implementation right.) - the http server accepts an unbounded number of connections. it'll just keep making goroutines as they come in. there's no way that I know of to apply back pressure. `net.Listener` is an interface you may implement as you choose. But in most cases a CPU limit is more than sufficient backpressure.
  - chrsig 4y ago
    > This is a reasonable default - probably the only reasonable default. `TeeReader` and `MultiReader` are easily available if you want to spare the memory. (But you're right that the converse on the ResponseWriter isn't true, it's much more difficult to get your own implementation right.) Fair enough > `net.Listener` is an interface you may implement as you choose Good point -- thanks, I hadn't considered to try and bound connections using a listener implementation. > But in most cases a CPU limit is more than sufficient backpressure. In many cases it's probably fine - but not all. The issue with letting CPU be the back pressure is that it'll happen _after_ the connection is established. Unless I'm mistaken, if a load balancer can't connect, it'll usually try another host, but if it connects and times out...there may have been some side effects, so it can't try another host.
silentprog 4y ago
I was in the same camp as you a few years ago and also wrote an utility to handle this. Might be of any interest to you. https://github.com/barthr/web-util https://github.com/barthr/web-util And the blog post explaining it: https://bartfokker.com/posts/clean-handlers https://bartfokker.com/posts/clean-handlers
didip 4y ago
This is such a tiny problem to quibble about. Just create a helper function that indeed force you to return values. The default leaves you room to do harder things like custom chunk streaming.
- badhombres 4y ago
  They did in the article? They're not asking it to change, they're just expressing their opinion.
Someone 4y ago
So, the issue is that there’s an implicit contract that says something like “a handler must write to the ResponseWriter or call http.Error before returning”, but the compiler doesn’t enforce that contract. The proposed improvement makes some ways to accidentally not adhere to the contract more obvious to humans, but still doesn’t enforce the contract. I wonder whether there are languages that allow one to write a library that enforces such contracts at compile time. (One way to do that would be to pass the Request and the ResponseWriter together with a state machine that is hard-coded in the API as a single object to the handler, with the equivalents of w.Write and http.Error declaring their state transitions, and having a language functionality that requires such object to be in an end state of that state machine whenever the function returns)
- jayd16 4y ago
  Well its a bit of an issue with escape analysis to know for sure that something is or isn't called before the handler returns. Imagine a scenario where multiple threads get involved. That would be a lot to track. I don't think its a great pattern but just speaking hypothetically, if you ensure that only valid Response instances exist (because you organized the constructors to only make valid instances and nulls are invalid) you can force the user to return a valid instance.
  - Someone 4y ago
    Yes, but there’s related prior art. Firstly, Java requires the compiler can prove variables are initialized before first use, and there’s a precisely described, fairly restricted algorithm that must be able to do that. Similarly, Rust can’t correctly accept all code without ownership issues, so it supports only a subset of such code (in this case, AFAIK, that set is defined by the compiler, and growing over time) In both cases the language designers on purpose limited what programs are valid so that the compiler can enforce certain constraints, eradicating a class of bugs. I wonder whether there’s a language that works similar for this kind of constraints. C++ destructors can do it for simple things such as “you must close this file before returning”, but that’s because the compiler will insert the call where needed. Enforcing “you must call f before you ever call g” also is doable in simple cases: have f return a value of a type that must be passed to g or an object foo on which one g is implemented as a member: foo.g(). I’m not aware of any language that allows for more complex state transitions, though. Edit: I think Rust can do (¿most of?) this by having a function take ownership of an object passed in, thus preventing further calls to it, and returning a new object on which only the then permissible calls can be made. I’m not sure one can require that to end with a ‘stopping state’ object, though.
timvisee 4y ago
With Rust you could write a function that consumes the HTTP client, not allowing you use it after, resulting in a compiler error for your example. In Go you could write a function taking the HTTP client reference and setting it to null. This would at least produce a runtime error if it ever comes across the path rather than doing something else that is unexpected.
binwiederhier 4y ago
I agree that they are awkward. I've solved it similarly by defining a custom handleFunc [1] and a custom errHTTP struct [2] which contains the HTTP response code and even more detailed error codes. It is very nice to work with. Like so: if err == util.ErrLimitReached { return errHTTPEntityTooLargeAttachmentTooLarge } Or so: if err != nil { return wrapErrHTTP(errHTTPBadRequestActionsInvalid, err.Error()) } [1] https://github.com/binwiederhier/ntfy/blob/main/server/server.go#L56 https://github.com/binwiederhier/ntfy/blob/main/server/serve... [2] https://github.com/binwiederhier/ntfy/blob/main/server/errors.go https://github.com/binwiederhier/ntfy/blob/main/server/error...
blablabla123 4y ago
Seems a rather theoretical problem to me because usually an external library is used. Also in the underlying http response first the return code is written before the body. This matters a lot when for instance copying a buffer into the response
adontz 4y ago
To me it seems like a problem induced by the way Go handles errors. With exceptions it would be something like try { // Some dangerous code which may throw/raise w.Write([]byte("Hello, World!")) } catch( FileException ) { w.Write([]byte("Screw your file!")) } catch( ... ) { w.Write([]byte("Screw you in general!")) } and no such problem
- randomdata 4y ago
  There is nothing special about errors, though. This same problem is present for any kind of branching operation. age := getUserAge() if age < 18 { w.Write([]byte("You are too young")) // Forgot to return here... } w.Write([]byte("<insert adult content>"))
- laumars 4y ago
  Putting aside discussions about Gos error handing and opinions about try and catch blocks, this specific problem (wrt the HTTP handler) can emulate the same code you’ve got above using a defer block. var err error defer func() { if err != nil { w.Write([]byte(err.Error()) } }()
jayd16 4y ago
Maybe this is a subtlety of Go I'm not aware of, but at least in most languages, the issue with the explicit return value is that you must first return before you can send the value. By being able to call into the response object, the handler is able to send to the client immediately, and then the handler writer can do other bookkeeping before exiting the handler. With a return value, work would need to be fire off asynchronously before the return value is sent.
donatj 4y ago
So the problem with these "good" examples is that they expose the end user to Errors, messages likely not written by you or your team and moreso not intended for general consumption - you need to be really careful about doing so because an unexpected error could expose lots of troubling things like credentials and general information about your system you'd rather not expose. We've got a general rule of thumb never to expose the message of an Error to the end user.
- collinvandyck76 4y ago
  Yep, that's why you'd want the error handler to try and type assert the error to something like an HTTPError where that's an interface you control. If it is an HTTPError, then you can "trust" the code/message and write that back to the user. Otherwise, 500 with a generic response body.
weltensturm 4y ago
Why is it http.Error(w, ...) and not w.Write(http.Error(...))? The latter would be way more clear in showing that nothing magical happens and a return (if wanted) is still necessary (I don't write Go, so I don't know the conventions)
- tptacek 4y ago
  `http.Error` is a convenience for writing an error message and setting the code. You can just use `w.Write` if you want. The reason `Error` isn't part of the interface signature for `ResponseWriter` is presumably that everything that implements `ResponseWriter` would have to duplicate the logic. Either way, it wouldn't address the bug here.
AtNightWeCode 4y ago
Surely one can make the exact same mistake with Echo? if id == "" { fmt.Errorf("get album: missing id") }
n4jm4 4y ago
I've made that same mistake before. And not just in HTTP handlers. There is a case to be made for a linter rule that warns when a conditional evaluates an error value, without presenting a return path.
jamal-kumar 4y ago
Well yeah, the default stdlib HTTP handlers are very basic. The cool thing is that the standard library is replete enough to build tons on top of that, though. CORS headers using nothing but stdlib for example: origin := http.StripPrefix("/", http.FileServer(http.Dir("www"))) wrapped := http.HandlerFunc(func(writer http.ResponseWriter, req *http.Request) { writer.Header().Set("Access-Control-Allow-Origin", "whatever.com") writer.Header().Set("Access-Control-Allow-Methods", "POST, GET, OPTIONS, PUT, DELETE") writer.Header(). Set("Access-Control-Allow-Headers", "Accept, Content-Type, Content-Length, Accept-Encoding, X-CSRF-Token, Authorization") origin.ServeHTTP(writer, req) }) http.Handle("/", wrapped) You end up writing a ton of boilerplate like this if you're sticking with the standard library, but having a ton of awareness and control over every aspect of what you're doing is really just so easy in this language. If I don't understand anything in the standard library I just go to the definition of it in the golang source code and usually end up figuring out what I need. It's really impressive how much you can get done without using any packages if you're writing anything net-facing that just needs to do one thing and do it well.
- twblalock 4y ago
  This is a good example of how standard libraries should be. They are fine for simple use cases, and they provide the basic primitives for more complex third-party libraries to build on top of. Stuff like basic HTTP functionality. Anything more complex than the example can be streamlined or abstracted away by a third-party library. People get very opinionated about how those kinds of abstractions should work, so it's probably better for the core Go team not to pick a model and spend time building it.
vrnvu 4y ago
I dislike the example. The author says that because we have a complex function, and the HTTP handler is designed the way it is, we cannot avoid running into problems. I disagree. This are two separate things. You could take a more functional approach and separate the main logic and the side effect. Which is writing the response. func aMoreAdvancedHandler(w http.ResponseWriter, r *http.Request) { res, err := helper() if err { http.Error(w, err.Error(), err.Status()) } w.write(res) } func helper() ([]byte, err error) { // todo } Where now the helper function has the problem of dealing with multiple operations that can potentially fail. Which is exactly the problem in the example. Not the HTTP handler itself... For example, we could use the defer function call with a named error to check if something fails and guarantee that we always return an appropriate error. Similar to the pattern used to free resources... I don't know how is this commonly called. But again, the problem is the helper function not the handler. I don't want to use the FP terminology, probably most people are familiar with this pseudo-code: func helper() (res, err) { return foo().bar().baz() } Where the chain foo, bar, baz will continue to be executed if all the calls are success, but early terminate if an error occurs. So, where is the problem?
- FpUser 4y ago
  I very much prefer this approach as well. Way better than endless chaining of this().that().repeat().million().times().
- kissgyorgy 4y ago
  It's very funny that you trying to invalidate the problem OP is explaining and you made the exact same mistake in 10 lines of code :) You forgot to return from the error handling branch.
deleted 4y ago
[deleted]
lhorie 4y ago
IMHO this boils down to purity vs side effects. Given a black box function, it's a lot easier to reason about what it did if it behaves in a pure manner returning some data structure that represents some form of state, than passing some mutable complex real world thing into it and then trying to inspect what the thing did after the fact. But the matter of purity vs side effects is completely orthogonal to the default HTTP handler design. The handler provides the inputs and the output sinks that represent the "real world". But it's entirely up to you how you manage the data therein. You can continue to procedurally consume these input/output entities turtles all the way down, or you can work in a functional style until you are actually ready to flush the result of the computation down the writer.
skybrian 4y ago
There's a subtle problem in the proposed solution: if an error happens after you already started writing the response, you can't change the http status code. So, what should you do with an error that happens while writing the HTTP response itself? Maybe keep statistics on how often it happens, in case happens often enough that it seems relevant for debugging. But there's no good way to report an error to the client because the network connection is likely broken. If you're not going to keep statistics, dropping errors from writing an http response is reasonable and arguably correct.
zxcvbn4038 4y ago
Who cares? Rando with a blog, maybe he'll like nodejs better.
oppositelock 4y ago
The low level HTTP library is phenomenal for building more friendly packages on top, think of it as assembly language for HTTP in go. What isn't obvious about that Go http design is that it's very parallel. Your callback gets invoked in a goroutine for each request, so you can write easy to understand code, linearly, without worrying about things like promises or callbacks or message queues. I'm opinionated here, because I also think all API's should be built starting from a spec, such as OpenAPI, and to that end, I've written a library to do just that, generate models and echo handlers from API specs, and other contributors have added gin, chi, etc [1] 1: https://github.com/deepmap/oapi-codegen https://github.com/deepmap/oapi-codegen
asp_hornet 4y ago
I thought a “naked return” was when the return types were named?
m00x 4y ago
Editorialized title. The real title is: "I Don’t Like Go’s Default HTTP Handlers". Please edit the HN title to reflect the blog post.
tomohawk 4y ago
Errors important to the server should not be conflated with http responses to the client. There is sometimes a correlation between the two, but often not.
crabmusket 4y ago
For fairness's sake, I'd like to point out that Nodejs has the same behaviour. Helping a new developer create a web server, we fell into the bugs the author is talking about a few times. I was always quick to spot the problem with the benefit of experience, but a newbie debugging a "response was already sent" error was not the simplest thing in the world.
carom 4y ago
I am writing a large Go web application right now and I disagree with this. I do not want to return an error. I want to write an error page or an error JSON message depending on the handler. By the time you are deep in a web application you are copy and pasting all your error blocks anyway. This [1] is a sample of one of my functions. I honestly love Go for how consistent it is. I wrote a rudimentary static checker [2] to make sure that any function does not call both the jsonError() and errorPage() functions. This was quite a problem given how much I reuse code. I disagree with the author's take because it is incredibly obvious and quite easy to track down when you forget a return. You will have a big block of garbage in the middle of your page and JSON, and your unique error message should lead you directly to the line. My problem is actually in executing the templates at the end of the functions [1], when you finally write out the page. If that has an error, you can't really back track. I've been thinking about changing that to write to a temporary buffer which then writes to the http.ResponseWriter but haven't made the change yet. 1. https://gist.github.com/TACIXAT/24621013248e2d91bebd16c5a02a99f9 https://gist.github.com/TACIXAT/24621013248e2d91bebd16c5a02a... 2. https://gist.github.com/TACIXAT/1000e7d873f0a8a738b3e720b4a7684c https://gist.github.com/TACIXAT/1000e7d873f0a8a738b3e720b4a7...
Gnouc 4y ago
You may want to use https://github.com/orijtech/httperroryzer https://github.com/orijtech/httperroryzer for your CI.
tastysandwich 4y ago
I can see the problem, but, I don't know, it's just not something that's ever bothered me. How big are his handlers anyway!? They should be pretty small IMO... I've managed to write some pretty complicated REST services with just the standard HTTP library and Mux for routing. In fact I've never used any of the bigger libraries like Gin or Echo. While all of my Node JS colleagues are drowning in dependabot PRs we're just cruising by in the Golang team with our small handful of dependencies. Heaven!
throwaddzuzxd 4y ago
In my team we used Gin and migrated to Echo (because Gin didn't support conflicting routes at the time, like /foo/{variable} and /foo/bar) and we got to the same conclusion. Forgetting to return with Gin (and with net/http) is an issue that actually occurs, and we've been bitten by it more times than we care to admit. Of course it's not worth migrating to Echo just for that but it's good to know that some routers implement it differently, if it's something that bothers you.
LordHeini 4y ago
Returning errors from a http handler is not a good idea imho. Because http errors/responses can be too complex for the limited Golang error objects. You usually want an appropriate error code, a body with a message or set some header.
Matthias247 4y ago
The article points out a „pro“ of returning a response in handlers - but fails to miss any cons. However there are a few of them. One particularly important one is that the „return response“ variants lack extremely on the observability front - which then in the end leads to services which are hard to run in production. Eg let’s say I want to measure in my handler what „last byte latency“ is - or at least when the handler pushed the last byte into a tcp socket. With the Go approach it’s not a problem - we can wrap the handler in another handler which does emit metrics. With the „return response object“ it won’t work easily anymore. At the time the handler returns nothing actually has happened. The same thing applies for eg tracking the amount of concurrently executed requests to understand resource utilization (and starvation). No issue with the Go variant - hard to do with „function as a service“. It’s actually rather unfortunate that all Rust frameworks went down the „response object“ route and made it hard to add proper observability.

192 points by p5v 4y ago

I agree, I wouldn't have been against a required return, but the mitigation is relatively straightforward by using custom handlers like mentioned at the end, if needed. I really like Go's default HTTP handlers because it offers that straightforward flexibility. Nothing rocket-sciencey.

antihero 4y ago
Well yeah, I think it’s important to let you write bytes to the socket and close it when you want to, and if people are paranoid about making obvious, easy to spot bugs, they can use some abstraction to protect themselves.
masklinn 4y ago
> the mitigation is relatively straightforward by using custom handlers like mentioned at the end, if needed. Why not implement ServeHTTP on the custom handler though? That's not exactly difficult: type MyHandlerFunc func(w http.ResponseWriter, r *http.Request) error func (f MyHandlerFunc) ServeHTTP(w http.ResponseWriter, r *http.Request) { err := f(w, r) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) } } Then you can just register it with http.Handle("/hello", MyHandlerFunc(someHandler))
- andrewstuart2 4y ago
  There are definitely edge cases here (like what if the handler already wrote a body and it was sent on the wire, etc), but this is absolutely the pattern to use, and one of my favorite parts of a type system that allows methods on function types.
  - catlifeonmars 4y ago
    Yes, or what if you want to upgrade the connection to a websocket?
- skrtskrt 4y ago
  Actually a bunch of router/web helper libraries do this in different ways which makes it a huge pain to compose middleware from different libraries. Even Mux's middleware library is not consistent in how it does this for different middlewares so they have to be applied different ways or even have wrapper functions written
  - catlifeonmars 4y ago
    This. Middlewares typically expect to wrap an http.Handler so you lose all the middleware composition ergonomics by adding a return value to your custom handler signature
  - masklinn 4y ago
    Here it just implements the standard library’s interface rather than use some sort of conversion closure adapter, there’s nothing special to it.
andrewstuart2 4y ago
The beauty is in the simplicity. `http.Handler` is all about what is needed to handle HTTP and nothing else. It doesn't intend to make things pretty, or safe, or anything really, except to express the minimum surface area required to interact with HTTP while abstracting the commonalities. Beyond that, as the article mentions, it's quite easy to abstract and implement your own sugar layer.

mrweasel 4y ago

It's not something I even thought about, but I guess I see the point, I just don't agree. In the HTTP handlers it makes sense that you don't have return values, because: What would you do with that value exactly? The HTTP handler should always ensure that "something" is returned, at the very least a return code. Once something has been returned to the client, can you really argue that there's an error? There's a point to the risk of missing return value, you just run the risk of complicating the request flow by adding it. I'd argue that most people would view the code path as having ended, once the client receives a response. The request shouldn't really continue on the server end after that. There can be background processing, go routines or something like that, but that's a bit different, as it doesn't involve the original requests-response.

jerf 4y ago
"In the HTTP handlers it makes sense that you don't have return values, because: What would you do with that value exactly?" I agree that the baseline net/http implementation is correct. There is no appropriate default error (return) handler that the framework could implement, at the level it lives at, that would be correct and wouldn't be limiting. However I very often in my own Go code immediately create an abstraction that does exactly this, because it is a good default for everything I'm doing. It just isn't suitable for everything (e.g., what do you do with this in the case of a protocol upgrade to websockets? the HTTP handlers need to be able to just terminate gracefully without being forced to "return" something). What I would say is, net/http is a little too low level to directly use. You don't need a full framework necessarily, though if you want to use one that does this more power to you. But if you understand HTTP, it doesn't take much wrapping to make net/http useful for "normal" APIs or websites. It takes a little, though.
- aniforprez 4y ago
  "net/http" doesn't seem to come with a lot of great defaults that almost every web app would use such as named path parameters which you'd need to write a bunch of code and regex to extract, middleware, routing based on HTTP method etc. This is all stuff you'd have to implement anyway I'd much prefer using something that's an abstraction of it like Gin, Chi, Echo and the like. They're fairly performative and add very little overhead on top of the standard package

dralley 4y ago

I like the way Axum for Rust handles this. All response handlers return something that implements IntoResponse. Lots of types implement IntoResponse, thus making the simple cases really streamlined. async fn create_user( Json(payload): Json<CreateUser>, ) -> impl IntoResponse { let user = User { id: 1337, username: payload.username, }; // this will be converted into a JSON response // with a status code of `201 Created` (StatusCode::CREATED, Json(user)) } If the handler is complex enough that branches can't all return the same type, you can just add .into_response() to each return site, and now you're just returning a response, which also implements IntoResponse (as a no-op). This avoids the problem discussed in this article while also avoiding much of the boilerplate of manually building responses.

masklinn 4y ago
I’ve not yet gotten to use it, but I understand Axum also implements IntoResponse for Result<A: IntoResponse, B: IntoResponse>, which is very convenient for the sort of error code of the above. It’s one of my biggest annoyance with Warp: it only implements Reply for `Result<T, http::error::Error>`, so if you have a faillible handler with more complicated error types (e.g. with a json body, or even a 200 response for RPC handlers) you’re in the awkward position of having to join the paths back (usually by converting everything to a Response by hand), which makes mistakes easier.
- zaarn 4y ago
  Correct, I spent about half an hour implementing IntoResponse for my own project's error type, but now I can just use normal error handling and I get presented with an error page on any issues. It's quite magic (not really, very streamlined).
pram 4y ago
That is what Gin does.
- maccard 4y ago
  Gin is excellent. It's the right balance of batteries included, sensible defaults, and black magic

halfmatthalfcat 4y ago

This isn't so much a critique of the HTTP handlers themselves but relying on (and stuffing a ton of shit into) context as a way of dealing with complex request/response patterns was a major headache. Trying to implement a circuit breaker against multiple upstreams for a single request in Go was a nightmare.

morelisp 4y ago
People put way too much stuff in the context (whether that's the req.Context() or the gin.Context or whatever). It should only be for things that are request-scoped and need to cross middlewares that aren't aware of them. Most such middlewares (logging, metrics, last-ditch error handling) get put at the front of the chain anyway. A circuit-breaker is basically by definition not request-scoped, so should not go in the context.

boredumb 4y ago

func (api \*App) HandleOnError(w http.ResponseWriter, err error, status int, context string) bool { if err != nil { api.logging.Error(err, context) http.Error(w, err.Error(), http.StatusInternalServerError) return true } return false } and then it's used in the handler funcs if api.HandleOnError(w, err, http.StatusInternalServerError, "Getting stuff for things") { return } Makes the logging verbose and handling always properly taken care of, makes the error handling in handlers verbose enough from the caller but abstract enough to not be a majority of the handlers body.

ainar-g 4y ago

Another possible extension of that design is using error types for HTTP codes. Something like: type HTTPError struct { Err error Code int } And then, with a couple of wrappers like this: func errorNotFound(err error) (wrapped error) { return &HTTPError{ Err: err, Code: http.StatusNotFound, } } you could do something like this: return errorNotFound(err) or this: return errorInternal(err) // 500 Internal Server Error

_adamb 4y ago
I usually create an abstraction like this in an API to require all returned errors to come with some sort of machine readable code. You either have to classify this response with an existing code in the list or add a new one. That list then gets generated as part of the documentation so consumers are getting possible error states straight from the code (without having to look at said code).
marcus_holmes 4y ago
I do similar: I create a `handleError` function that deals with the error (logs it, reports it, redirects to the appropriate page), and call that in a return statement. It's like 5 minute's work to code up, and I get to set whatever logging destination and redirect logic works best for my project.

JustSomeNobody 4y ago

They're not functions, they're procedures. Procedures don't return values. Write your code accordingly.

badhombres 4y ago

To me this reads as a positive to the default handlers, as it shows how easy it was to make it work into a format that works for the programmer's preference. I appreciated the solution to the complaint

deleted 4y ago

[deleted]

alectic 4y ago

Streamed-response use cases get weird/complicated if you have to return the response.

masklinn 4y ago
Sure but is that the usual / default? Why not have a second adapter?
deathanatos 4y ago
Not necessarily? The type used for Response can include a "body" type that supports streaming. E.g., body: AsyncRead in Rust. In Go I think it would resemble a channel that emitted the contents of the body. (As a library, though, you'd also want to make the simple non-streaming case of "just return this blob of bytes" simple for the consumer, too.)

alingse 4y ago

we have already do this in prod. type HandlerFuncErr func(w http.ResponseWriter, r http.Request) error func HandleErr(h HandlerFuncErr) http.HandlerFunc { return func(w http.ResponseWriter, r http.Request) { ... } }

chrsig 4y ago

I dont necessarily agree they should return a value, but I do have some issues with the api: - by default, you can't tell if a handler has already written to the body or emitted a status code. - by default, you can't read the body more than once - r.PostForm vs r.Form ...that they both exist, and that they're only available after a manual call to r.ParseForm - the http server accepts an unbounded number of connections. it'll just keep making goroutines as they come in. there's no way that I know of to apply back pressure. there's probably more that I grump about, but those are the ones at the top of my mind. for the most part it works well, and i can work around my gripes.

morelisp 4y ago
- by default, you can't read the body more than once This is a reasonable default - probably the only reasonable default. `TeeReader` and `MultiReader` are easily available if you want to spare the memory. (But you're right that the converse on the ResponseWriter isn't true, it's much more difficult to get your own implementation right.) - the http server accepts an unbounded number of connections. it'll just keep making goroutines as they come in. there's no way that I know of to apply back pressure. `net.Listener` is an interface you may implement as you choose. But in most cases a CPU limit is more than sufficient backpressure.
- chrsig 4y ago
  > This is a reasonable default - probably the only reasonable default. `TeeReader` and `MultiReader` are easily available if you want to spare the memory. (But you're right that the converse on the ResponseWriter isn't true, it's much more difficult to get your own implementation right.) Fair enough > `net.Listener` is an interface you may implement as you choose Good point -- thanks, I hadn't considered to try and bound connections using a listener implementation. > But in most cases a CPU limit is more than sufficient backpressure. In many cases it's probably fine - but not all. The issue with letting CPU be the back pressure is that it'll happen _after_ the connection is established. Unless I'm mistaken, if a load balancer can't connect, it'll usually try another host, but if it connects and times out...there may have been some side effects, so it can't try another host.

silentprog 4y ago

I was in the same camp as you a few years ago and also wrote an utility to handle this. Might be of any interest to you. https://github.com/barthr/web-util https://github.com/barthr/web-util And the blog post explaining it: https://bartfokker.com/posts/clean-handlers https://bartfokker.com/posts/clean-handlers

didip 4y ago

This is such a tiny problem to quibble about. Just create a helper function that indeed force you to return values. The default leaves you room to do harder things like custom chunk streaming.

badhombres 4y ago
They did in the article? They're not asking it to change, they're just expressing their opinion.

Someone 4y ago

So, the issue is that there’s an implicit contract that says something like “a handler must write to the ResponseWriter or call http.Error before returning”, but the compiler doesn’t enforce that contract. The proposed improvement makes some ways to accidentally not adhere to the contract more obvious to humans, but still doesn’t enforce the contract. I wonder whether there are languages that allow one to write a library that enforces such contracts at compile time. (One way to do that would be to pass the Request and the ResponseWriter together with a state machine that is hard-coded in the API as a single object to the handler, with the equivalents of w.Write and http.Error declaring their state transitions, and having a language functionality that requires such object to be in an end state of that state machine whenever the function returns)

jayd16 4y ago
Well its a bit of an issue with escape analysis to know for sure that something is or isn't called before the handler returns. Imagine a scenario where multiple threads get involved. That would be a lot to track. I don't think its a great pattern but just speaking hypothetically, if you ensure that only valid Response instances exist (because you organized the constructors to only make valid instances and nulls are invalid) you can force the user to return a valid instance.
- Someone 4y ago
  Yes, but there’s related prior art. Firstly, Java requires the compiler can prove variables are initialized before first use, and there’s a precisely described, fairly restricted algorithm that must be able to do that. Similarly, Rust can’t correctly accept all code without ownership issues, so it supports only a subset of such code (in this case, AFAIK, that set is defined by the compiler, and growing over time) In both cases the language designers on purpose limited what programs are valid so that the compiler can enforce certain constraints, eradicating a class of bugs. I wonder whether there’s a language that works similar for this kind of constraints. C++ destructors can do it for simple things such as “you must close this file before returning”, but that’s because the compiler will insert the call where needed. Enforcing “you must call f before you ever call g” also is doable in simple cases: have f return a value of a type that must be passed to g or an object foo on which one g is implemented as a member: foo.g(). I’m not aware of any language that allows for more complex state transitions, though. Edit: I think Rust can do (¿most of?) this by having a function take ownership of an object passed in, thus preventing further calls to it, and returning a new object on which only the then permissible calls can be made. I’m not sure one can require that to end with a ‘stopping state’ object, though.

timvisee 4y ago

With Rust you could write a function that consumes the HTTP client, not allowing you use it after, resulting in a compiler error for your example. In Go you could write a function taking the HTTP client reference and setting it to null. This would at least produce a runtime error if it ever comes across the path rather than doing something else that is unexpected.

binwiederhier 4y ago

I agree that they are awkward. I've solved it similarly by defining a custom handleFunc [1] and a custom errHTTP struct [2] which contains the HTTP response code and even more detailed error codes. It is very nice to work with. Like so: if err == util.ErrLimitReached { return errHTTPEntityTooLargeAttachmentTooLarge } Or so: if err != nil { return wrapErrHTTP(errHTTPBadRequestActionsInvalid, err.Error()) } [1] https://github.com/binwiederhier/ntfy/blob/main/server/server.go#L56 https://github.com/binwiederhier/ntfy/blob/main/server/serve... [2] https://github.com/binwiederhier/ntfy/blob/main/server/errors.go https://github.com/binwiederhier/ntfy/blob/main/server/error...

blablabla123 4y ago

Seems a rather theoretical problem to me because usually an external library is used. Also in the underlying http response first the return code is written before the body. This matters a lot when for instance copying a buffer into the response

adontz 4y ago

To me it seems like a problem induced by the way Go handles errors. With exceptions it would be something like try { // Some dangerous code which may throw/raise w.Write([]byte("Hello, World!")) } catch( FileException ) { w.Write([]byte("Screw your file!")) } catch( ... ) { w.Write([]byte("Screw you in general!")) } and no such problem

randomdata 4y ago
There is nothing special about errors, though. This same problem is present for any kind of branching operation. age := getUserAge() if age < 18 { w.Write([]byte("You are too young")) // Forgot to return here... } w.Write([]byte("<insert adult content>"))
laumars 4y ago
Putting aside discussions about Gos error handing and opinions about try and catch blocks, this specific problem (wrt the HTTP handler) can emulate the same code you’ve got above using a defer block. var err error defer func() { if err != nil { w.Write([]byte(err.Error()) } }()

jayd16 4y ago

Maybe this is a subtlety of Go I'm not aware of, but at least in most languages, the issue with the explicit return value is that you must first return before you can send the value. By being able to call into the response object, the handler is able to send to the client immediately, and then the handler writer can do other bookkeeping before exiting the handler. With a return value, work would need to be fire off asynchronously before the return value is sent.

donatj 4y ago

So the problem with these "good" examples is that they expose the end user to Errors, messages likely not written by you or your team and moreso not intended for general consumption - you need to be really careful about doing so because an unexpected error could expose lots of troubling things like credentials and general information about your system you'd rather not expose. We've got a general rule of thumb never to expose the message of an Error to the end user.

collinvandyck76 4y ago
Yep, that's why you'd want the error handler to try and type assert the error to something like an HTTPError where that's an interface you control. If it is an HTTPError, then you can "trust" the code/message and write that back to the user. Otherwise, 500 with a generic response body.

weltensturm 4y ago

Why is it http.Error(w, ...) and not w.Write(http.Error(...))? The latter would be way more clear in showing that nothing magical happens and a return (if wanted) is still necessary (I don't write Go, so I don't know the conventions)

tptacek 4y ago
`http.Error` is a convenience for writing an error message and setting the code. You can just use `w.Write` if you want. The reason `Error` isn't part of the interface signature for `ResponseWriter` is presumably that everything that implements `ResponseWriter` would have to duplicate the logic. Either way, it wouldn't address the bug here.

AtNightWeCode 4y ago

Surely one can make the exact same mistake with Echo? if id == "" { fmt.Errorf("get album: missing id") }

n4jm4 4y ago

I've made that same mistake before. And not just in HTTP handlers. There is a case to be made for a linter rule that warns when a conditional evaluates an error value, without presenting a return path.

jamal-kumar 4y ago

Well yeah, the default stdlib HTTP handlers are very basic. The cool thing is that the standard library is replete enough to build tons on top of that, though. CORS headers using nothing but stdlib for example: origin := http.StripPrefix("/", http.FileServer(http.Dir("www"))) wrapped := http.HandlerFunc(func(writer http.ResponseWriter, req *http.Request) { writer.Header().Set("Access-Control-Allow-Origin", "whatever.com") writer.Header().Set("Access-Control-Allow-Methods", "POST, GET, OPTIONS, PUT, DELETE") writer.Header(). Set("Access-Control-Allow-Headers", "Accept, Content-Type, Content-Length, Accept-Encoding, X-CSRF-Token, Authorization") origin.ServeHTTP(writer, req) }) http.Handle("/", wrapped) You end up writing a ton of boilerplate like this if you're sticking with the standard library, but having a ton of awareness and control over every aspect of what you're doing is really just so easy in this language. If I don't understand anything in the standard library I just go to the definition of it in the golang source code and usually end up figuring out what I need. It's really impressive how much you can get done without using any packages if you're writing anything net-facing that just needs to do one thing and do it well.

twblalock 4y ago
This is a good example of how standard libraries should be. They are fine for simple use cases, and they provide the basic primitives for more complex third-party libraries to build on top of. Stuff like basic HTTP functionality. Anything more complex than the example can be streamlined or abstracted away by a third-party library. People get very opinionated about how those kinds of abstractions should work, so it's probably better for the core Go team not to pick a model and spend time building it.

vrnvu 4y ago

I dislike the example. The author says that because we have a complex function, and the HTTP handler is designed the way it is, we cannot avoid running into problems. I disagree. This are two separate things. You could take a more functional approach and separate the main logic and the side effect. Which is writing the response. func aMoreAdvancedHandler(w http.ResponseWriter, r *http.Request) { res, err := helper() if err { http.Error(w, err.Error(), err.Status()) } w.write(res) } func helper() ([]byte, err error) { // todo } Where now the helper function has the problem of dealing with multiple operations that can potentially fail. Which is exactly the problem in the example. Not the HTTP handler itself... For example, we could use the defer function call with a named error to check if something fails and guarantee that we always return an appropriate error. Similar to the pattern used to free resources... I don't know how is this commonly called. But again, the problem is the helper function not the handler. I don't want to use the FP terminology, probably most people are familiar with this pseudo-code: func helper() (res, err) { return foo().bar().baz() } Where the chain foo, bar, baz will continue to be executed if all the calls are success, but early terminate if an error occurs. So, where is the problem?

FpUser 4y ago
I very much prefer this approach as well. Way better than endless chaining of this().that().repeat().million().times().
kissgyorgy 4y ago
It's very funny that you trying to invalidate the problem OP is explaining and you made the exact same mistake in 10 lines of code :) You forgot to return from the error handling branch.

deleted 4y ago

[deleted]

lhorie 4y ago

IMHO this boils down to purity vs side effects. Given a black box function, it's a lot easier to reason about what it did if it behaves in a pure manner returning some data structure that represents some form of state, than passing some mutable complex real world thing into it and then trying to inspect what the thing did after the fact. But the matter of purity vs side effects is completely orthogonal to the default HTTP handler design. The handler provides the inputs and the output sinks that represent the "real world". But it's entirely up to you how you manage the data therein. You can continue to procedurally consume these input/output entities turtles all the way down, or you can work in a functional style until you are actually ready to flush the result of the computation down the writer.

skybrian 4y ago

There's a subtle problem in the proposed solution: if an error happens after you already started writing the response, you can't change the http status code. So, what should you do with an error that happens while writing the HTTP response itself? Maybe keep statistics on how often it happens, in case happens often enough that it seems relevant for debugging. But there's no good way to report an error to the client because the network connection is likely broken. If you're not going to keep statistics, dropping errors from writing an http response is reasonable and arguably correct.

zxcvbn4038 4y ago

Who cares? Rando with a blog, maybe he'll like nodejs better.

oppositelock 4y ago

The low level HTTP library is phenomenal for building more friendly packages on top, think of it as assembly language for HTTP in go. What isn't obvious about that Go http design is that it's very parallel. Your callback gets invoked in a goroutine for each request, so you can write easy to understand code, linearly, without worrying about things like promises or callbacks or message queues. I'm opinionated here, because I also think all API's should be built starting from a spec, such as OpenAPI, and to that end, I've written a library to do just that, generate models and echo handlers from API specs, and other contributors have added gin, chi, etc [1] 1: https://github.com/deepmap/oapi-codegen https://github.com/deepmap/oapi-codegen

asp_hornet 4y ago

I thought a “naked return” was when the return types were named?

m00x 4y ago

Editorialized title. The real title is: "I Don’t Like Go’s Default HTTP Handlers". Please edit the HN title to reflect the blog post.

tomohawk 4y ago

Errors important to the server should not be conflated with http responses to the client. There is sometimes a correlation between the two, but often not.

crabmusket 4y ago

For fairness's sake, I'd like to point out that Nodejs has the same behaviour. Helping a new developer create a web server, we fell into the bugs the author is talking about a few times. I was always quick to spot the problem with the benefit of experience, but a newbie debugging a "response was already sent" error was not the simplest thing in the world.

carom 4y ago

I am writing a large Go web application right now and I disagree with this. I do not want to return an error. I want to write an error page or an error JSON message depending on the handler. By the time you are deep in a web application you are copy and pasting all your error blocks anyway. This [1] is a sample of one of my functions. I honestly love Go for how consistent it is. I wrote a rudimentary static checker [2] to make sure that any function does not call both the jsonError() and errorPage() functions. This was quite a problem given how much I reuse code. I disagree with the author's take because it is incredibly obvious and quite easy to track down when you forget a return. You will have a big block of garbage in the middle of your page and JSON, and your unique error message should lead you directly to the line. My problem is actually in executing the templates at the end of the functions [1], when you finally write out the page. If that has an error, you can't really back track. I've been thinking about changing that to write to a temporary buffer which then writes to the http.ResponseWriter but haven't made the change yet. 1. https://gist.github.com/TACIXAT/24621013248e2d91bebd16c5a02a99f9 https://gist.github.com/TACIXAT/24621013248e2d91bebd16c5a02a... 2. https://gist.github.com/TACIXAT/1000e7d873f0a8a738b3e720b4a7684c https://gist.github.com/TACIXAT/1000e7d873f0a8a738b3e720b4a7...

Gnouc 4y ago

You may want to use https://github.com/orijtech/httperroryzer https://github.com/orijtech/httperroryzer for your CI.

tastysandwich 4y ago

I can see the problem, but, I don't know, it's just not something that's ever bothered me. How big are his handlers anyway!? They should be pretty small IMO... I've managed to write some pretty complicated REST services with just the standard HTTP library and Mux for routing. In fact I've never used any of the bigger libraries like Gin or Echo. While all of my Node JS colleagues are drowning in dependabot PRs we're just cruising by in the Golang team with our small handful of dependencies. Heaven!

throwaddzuzxd 4y ago

In my team we used Gin and migrated to Echo (because Gin didn't support conflicting routes at the time, like /foo/{variable} and /foo/bar) and we got to the same conclusion. Forgetting to return with Gin (and with net/http) is an issue that actually occurs, and we've been bitten by it more times than we care to admit. Of course it's not worth migrating to Echo just for that but it's good to know that some routers implement it differently, if it's something that bothers you.

LordHeini 4y ago

Returning errors from a http handler is not a good idea imho. Because http errors/responses can be too complex for the limited Golang error objects. You usually want an appropriate error code, a body with a message or set some header.

Matthias247 4y ago

The article points out a „pro“ of returning a response in handlers - but fails to miss any cons. However there are a few of them. One particularly important one is that the „return response“ variants lack extremely on the observability front - which then in the end leads to services which are hard to run in production. Eg let’s say I want to measure in my handler what „last byte latency“ is - or at least when the handler pushed the last byte into a tcp socket. With the Go approach it’s not a problem - we can wrap the handler in another handler which does emit metrics. With the „return response object“ it won’t work easily anymore. At the time the handler returns nothing actually has happened. The same thing applies for eg tracking the amount of concurrently executed requests to understand resource utilization (and starvation). No issue with the Go variant - hard to do with „function as a service“. It’s actually rather unfortunate that all Rust frameworks went down the „response object“ route and made it hard to add proper observability.