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:
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")
}
In 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.
It leaves very little to the imagination as to whether or not ServeHTTP works, which is nice.
Complexity comes from generating requests and parsing the responses, and that is what leads to the desire to factor things out -- test the functions with their native data types instead of http.Request and http.Response. I think most people choose to factor things out to make that possible, but in the simplest of simple cases, many people just use httptest. It gets the job done.
I don't think it's poor to test http handling either, as a coarse grained integration test.
The problem I've seen is over-dependence on writing unit tests with mocks instead of biting the bullet and properly testing all the boundaries. I have seen folk end up with 1000+ tests, of which most are useless because the mocks make far too many assumptions, but are necessary because of the layer coupling.
This was mostly in Node though, where mocking the request/response gets done inconsistently, per framework. Go might have better tooling in that regard, and maybe that sways the equation a bit. IMO there's still merit to decoupling if there's any feasibility of e.g. migrating to GraphQL or another protocol without having to undergo an entire re-write.
> I don't think it's poor to test http handling either, as a coarse grained integration test.
Sorry to spring a mostly-unrelated question on you about this, but why do you call this an integration test? I recently interviewed three candidates in a row that described their tests in this way, and I thought it was odd, and now I see many people in this thread doing it also.
I would call this a functional or behavioral test. For me a key aspect of an integration test is that there's something "real" on at least two "sides" - otherwise what is it testing integration with? Is this some side-effect of a generation growing up with Spring's integration testing framework being used for all black-box testing?
(I will not comment about how often I see people referring to all test doubles as "mocks", as I have largely given up trying to bring clarity here...)
The reality is that I've heard unit, integration and e2e almost entirely used interchangeably, maybe except the former and latter. I don't think trying to nail down the terms to something concrete is necessarily a useful exercise. Attempts to do so, imo, make subjective sense in the terms of the individual's stack/deployment scenario.
To me, it's a contextual term much like 'single responsibility'. In this case, the two "sides" of an integration test are present. A consumer issues a request and a provider responds accordingly. The tests would ascertain that with variations to the client request, the provider behaves in the expected manner.
At which point you might point out that this sounds like an e2e test, but actually using the client web app, for example, might involve far more than a simple http client/library - in no small part because the provider can easily run a simple consumer in memory and avoid the network entirely. E2e tests tend to be far more fragile, so from the perspective of achieving practical continuous deployment, it's a useful distinction.
integration tests in this instance: varying HTTP requests (infrastructure layer) provoke correct behaviour in application layer.
e2e: intended client issues http requests under the correct conditions, which provokes certain provider behaviour, which client then actually utilises correctly.
This, to me, is why the most important part of testing is understanding the boundaries of the tests. Not worrying about their names.
Just want to address (1) quickly. As you've mentioned at the end of the parenthesized note, the reason I did not use `defer` here is to avoid the lock staying across the rest of the handler. I wanted to confine it to the datastore interaction.
Thinking more about this now and having read the comments, I'm considering to just hide the lock in TaskStore and avoid all these explicit locks/unlocks in handlers; it seems like it will avoid some confusion for folks reading the example (as well as quite a few lines of code!), and my goal here is really the HTTP server logic. I prefer to deflect any attention from TaskStore in this series of posts.
If you only want to hold the lock for a portion of a function, pull that part out into its own function. Makes the code much easier to reason about. I consider .Unlock() without defer to be a code smell in nearly all cases.
Would 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.
Not 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.
> 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?
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:
You might like: 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:
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: Then in your handler function: 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.