various fixes, and idiomatic Go #5
Conversation
Change-type: minor Signed-off-by: Alex Bucknall <alex.bucknall@gmail.com>
Change-type: minor Signed-off-by: Alex Bucknall <alex.bucknall@gmail.com>
Change-type: minor Signed-off-by: Alex Bucknall <alex.bucknall@gmail.com>
|
LGTM |
| card, err := setupNotecard(transport) | ||
| if err != nil { | ||
| log.Printf("while setting up notecard: %v", err) | ||
| } | ||
| defer card.Close() |
There was a problem hiding this comment.
we don't really care given our lifecycle, but this is going to fail (panic even), if the setup above failed (because card will be nil).
| if s.card == nil || s.initError != nil { | ||
| handleError(w, s.initError, "while initialising notecard") | ||
| log.Fatal("Notecard not initialised, exiting...") | ||
| } |
There was a problem hiding this comment.
There's two stances here: either you trust that the caller have consistently and properly set the (s.card, s.initError) tuple, or you don't.
If you do, it means you only need to test either of them (any is fine, as each of them carry the meaning of success VS failure), so you only have to choose whether your prefer to test card or initError.
If you don't, they you need to test both of them, like you did. But then you need to go a bit further in the defensiveness. Since we can't trust the caller, it could very well be e.g. that they have set s.card but they have forgotten to set s.initError. In which case, your test is going to be true, but you're going to pass a nil error to handleError. So we would need to check that s.initError is not nil, etc..
Anyway, long story short, since we (2 programmers at arribada) are the caller in that case, I would choose to trust us, and I would change L32 to test only for s.initError.
probably the most important fix: the request body was never getting closed. AFAIR that leads to resource leaking (mem or file handlers, don't recall for sure).
also changed the error handling in the http server. in general, as a matter of security, you don't want to serve too much of the diagnostic to the client. So you log it for yourself, but you don't serve the details of the error.
also you usually only want to panic in rare cases: when you're debugging, or want you want to bring attention to yourself the dev that something that should never ever really happen happened (and you probably don't even want to do that on long lived entities like an HTTP server, that should always be up). But if the logging I added instead is not enough for you, we can of course revisit that.
we were JSON decoding, and JSON reencoding, the request body for no reason that I could see. so I removed that. And I think we can even do better, see my TODO in the code.
using a global var for the card context was ok in that case, but I still made it a bit more Go idiomatic, by making it part of a struct that implements ServeHTTP instead.
also note, you can't (well, you can, but it's useless) do a WriteHeader after a Write. Because as soon as you call Write, the response (and hence the header) has already been sent (with an implicit call to WriteHeader).
other than that, various cleanups to make it more Go idiomatic :)
it builds on balena, but I haven't been able to actually test it oc.