4 ms·
Good introduction. A few thoughts: 1) Be careful with locks in the form "x.Lock(); x.DoSomething(); x.Unlock()". If DoSomething panics, you will still be hol
by jrockway 6y ago
Good introduction. A few thoughts:
1) Be careful with locks in the form "x.Lock(); x.DoSomething(); x.Unlock()". If DoSomething panics, you will still be holding the lock, and that's pretty much the end of your program. ("x.Lock(); defer x.Unlock(); x.DoSomething()" avoids this problem, but obviously in the non-panic case, the lock is released at a different time than in this implementation. Additional tweaking is required.)
Generally I don't like locks in the request critical path because waiting for a lock is uncancelable, but in this very simple case it doesn't matter. For more complicated concurrency requirements, consider the difference between x.Lock()/x.Do()/x.Unlock vs. select { case x := <-ch: doSomethingWithX(x); case <-request.Context().Done(): error(request.Context().Err()) }. The channel wait can be cancelled when the user disconnects, or hits the stop button in the error, or the request timeout is reached.
2) Long if/else statements are harder to read than a switch statement. Instead of:
if(foo == "bar") {
// Bar
} else if (foo == "baz") {
// Baz
} else {
// Error
}
You might like:
switch(foo) {
case "bar":
// Bar
case "baz":
// Baz
default:
// Error
}
These are exactly semantically equivalent, and neither protect you at compile-time from forgetting a case, but there is slightly less visual noise. Worth considering.
3) I have always found that error handling in http.HandlerFunc-tions cumbersome. The author runs into this, with code like:
foo, err := Foo()
if err != nil {
http.Error(w, ...)
return
}
bar, err := Bar()
if err != nil {
http.Error(w, ...)
return
}
Basically, you end up writing the error handling code a number of times, and you have to do two things in the "err != nil" block, which is annoying. I prefer:
func DoTheActualThing() ([]byte, error) {
if everythingIsFine {
return []byte(`{"result":"it worked and you are cool"}`), nil
}
return nil, errors.New("not everything is okay, feels sad")
}
Then in your handler function:
func ServeHTTP(w http.ResponseWriter, req *http.Request) {
result, err := DoTheActualThing()
if err != nil {
http.Error(w, ...)
return
}
w.Header().Set("content-type", "application/json")
w.WriteHeader(http.StatusOK)
w.Write(result)
}
In this simple example, it doesn't matter, but when you do more than one thing that can cause an error, you'll like it better.
- klohto 6y agoWould you please expand more on your first point regarding using channels instead of Locks? It’s hard for me to wrap a head around it without practical example.
- deleted 6y ago[deleted]
- Philip-J-Fry 6y agoNot the OP but basically imagine that instead of locking a mutex to handle synchronised writes, you spawn a goroutine which just reads from a channel and writes the data. If that goroutine hasn't finished processing then the channel will be blocked, just like a mutex. So in your handler you can use a select statement to either write to the channel OR read from the request.Context().Done(). The request context only lives as long as the request. So if the connection drops or times out then the context gets cancelled and a value is pushed onto the done channel and your read is unblocked. Because you use a select statement then which ever operation unblocks first is what happens. If the write channel unblocks then you get to write your value. If your request context gets cancelled then you can report an error. The request context will always get cancelled eventually, unlike a mutex which will wait forever.
- makeworld 6y ago> Be careful with locks in the form "x.Lock(); x.DoSomething(); x.Unlock()". If DoSomething panics, you will still be holding the lock, and that's pretty much the end of your program. Interesting, thanks. But isn't panicking the end of your program anyway? Could you provide another example where no using defer causes problems?
- acrispino 6y agoNot necessarily. Panics can be recovered and the stdlib http server recovers panics from handlers.
- AWebOfBrown 6y agoIn the latter example, the question is really one of how tightly you wish to couple the application layer to that of the infrastructure (controller). Should the application logic be coupled to a http REST API (and thus map application errors to status codes etc), or does that belong in the controller? I don't disagree that it's more practical, initially, as you've described it. However, I think it's important to point out the tradeoff rather than presenting it as purely more efficient. I've seen this approach result in poor separation of concerns and bloated use cases (`DoTheActualThing`) which become tedious to refactor, albeit in other languages. One predictable side effect of the above, if you're working with junior engineers, is that they are likely going to write tests for the application logic with a dependency on the request / response as inputs, and asserting on status codes etc. I shudder to think how many lines of code I've read dedicated to mocking req/res that were never needed in the first place.