proposal: API change: redesign error handling
nsajko opened this issue · 8 comments
Currently not all errors are handled:
core/coffer.go:48:14: Error return value of `s.Initialise` is not checked
s.Initialise()
^
core/coffer_test.go:148:9: Error return value of `s.Rekey` is not checked
s.Rekey()
^
core/exit.go:67:19: Error return value of `memcall.Protect` is not checked
memcall.Protect(b.inner, memcall.ReadWrite)
^
enclave_test.go:76:9: Error return value of `e.Open` is not checked
e.Open()
^
memcall/memcall_test.go:48:6: Error return value of `Free` is not checked
Free(buffer)
^
memcall/memcall_unix.go:14:14: Error return value of `unix.Madvise` is not checked
unix.Madvise(b, unix.MADV_DONTDUMP)
^
memguard.go:10:26: Error return value of `memcall.DisableCoreDumps` is not checked
memcall.DisableCoreDumps()
^
Also, it would be better to have predicate error checking functions than sentinel error values, with no error types or values exported. For example: func IsBufferExpired(err error) bool. This would give you the most freedom going forward because of not unnecessarily exporting API, and also give users programmatic inspectionability of your errors.
As far as I can tell from that, none of those errors really need checking.
Not fully sure what you are proposing with the predicate checking functions. Some concrete examples would be helpful.
(Continuing from the pull request.)
this change seems to export lots of new things
It exports only a single predicate func for each sentinel error value that was previously exported. For example instead of var ErrBufferExpired that change/pull request exports a func IsBufferExpired(e error) bool predicate.
Regarding your suggestion to use methods instead of functions, I think that would complicate things all in all. In that case you would need to export a type (for example MemguardError), and because errors are conventionally returned as the built in error interface, the user would need to assert type from error to MemguardError to use the method. Also, exporting an error type is bad in itself as it means needlessly mantaining API compatibility.
One of the problems with sentinel error values like they are currently implemented in Memguard (as public variables) is that they are not constants, imagine a user doing something like core.ErrBufferExpired = nil. While that is probably only a formal possibility, I think it is terribly ugly.
The advantage with an approach like in the pull request is that the implementation can be drastically changed with no changes to the exported API necessary. For example, maybe you would want to have errors that can satisfy more than one predicate at the same time: no change to exported API necessary. Or maybe you would want to make unwrapping your errors possible (like in the latest blog post by rsc that shows changes coming in go 1.13: https://blog.golang.org/experiment ), again sentinel error values would just complicate things.
I’m curious, why is it better to abstract and wrap error value checking expressions in a function than direct expressions ?
@prologic That is not at all what I was trying to get across. Did you read my comment immediately preceding yours? The point is to have a better public API with more freedom to change the implementation.
@prologic That is not at all what I was trying to get across. Did you read my comment immediately preceding yours? The point is to have a better public API with more freedom to change the implementation.
Oh hey I mean no ill intend in my comment/question; I'm genuinely interested in pros/cons of both approaches for my own project too Bitcaks where I also expose a public error value (just one at this time). I'm also curious if you know which is more common amongst Gophers; exposing public error values or hiding it behind predicate public functions? Any good references/articles I can read?
no ill intent
Likewise :)
I'm also curious if you know which is more common amongst Gophers; exposing public error values or hiding it behind predicate public functions?
A very interesting question but I am afraid I do not have a good answer. A lot of libraries expose neither error values nor error types, not even predicates which take errors (then the user of the library is expected to just check if the error is nil, and print the Error() message otherwise). Error wrapping is also sometimes used, with standard library support for unwrapping in Go tip (coming in go 1.13).
An interesting case is the os package in the Go standard library. It exports all three of error variables, error types and error checking predicate funcs; I would love to know the history of that:
https://golang.org/pkg/os/#pkg-variables
https://golang.org/pkg/os/#IsExist
https://golang.org/pkg/os/#LinkError
https://golang.org/pkg/os/#SyscallError
Any good references/articles I can read?
Again, I would myself like a good answer to this question, but I think my views on error handling were somewhat shaped by reading Dave Cheney's blog. Sadly, as far as I remember there was not one article that sums it all up. The recent (upcoming in go 1.13) changes, design documents, problem overviews are probably a good read (I am TODO on that myself ;):
https://tip.golang.org/pkg/errors/
https://go.googlesource.com/proposal/+/master/design/29934-error-values.md
https://go.googlesource.com/proposal/+/master/design/go2draft-error-values-overview.md
https://go.googlesource.com/proposal/+/master/design/go2draft-error-inspection.md
This old blog post is also worth a read:
So I've read a few of the mentioned articles; especially throughly read the Golang glog on the subject. I've also gotten somewhat involved in the (somewhat questionable_) error handling proposals put forth for Go 2.0 (look for my comments!)
My take on all this thus far is the following:
Unwrap()looks like a nice addition- This leads me to believe maybe library authors should define important exported errors as types that implement
Wrap()
I'm still on the fence about "predicate functions" though as I kind of prefer explicitness here usually and I admit I'm struggling to identify a scenario where I would change the internals of what errors are to a degree that would affect a user of a library (example: I haven't thus far felt the need to change error values once defined in any of my librararies)
the user would need to assert type from error to MemguardError to use the method
You're right.
imagine a user doing something like core.ErrBufferExpired = nil
Honestly at that point the user deserves what they get.
maybe you would want to have errors that can satisfy more than one predicate at the same time
Interesting possibility, although it hasn't been needed so far.