In your code, the cache layer always returns a non-nil error, even when the value was found in the cache *and* the value was error-free. I don't think that's realistic. You're changing "error" from an indication of a problem, into auxiliary metadata about the value being returned (e.g. "this value came from the cache"). It means the consumer can no longer use "err != nil" to detect a problem, which is the most basic contract for error.
This can be fixed: https://go.dev/play/p/b_xLb06goXH But then you lose the provenance information for for error-free data (i.e. did it come from the cache or not?). And I still don't like the idea that a *real* error that happened can sometimes be wrapped in a cache error and sometimes not. I think that's because the cache error isn't really a caching error (the cache retrieved the value successfully!), but an auxiliary indication that this error value was retrieved via the cache. The error itself is really the same. The point of this thread was whether wrapping is necessary, or what the alternative approaches are. I would suggest here that you can avoid wrapping by following the way fs.PathError works, which is a typed error rather than a singleton: https://go.dev/play/p/a3cc4Watwie That still doesn't give you the metadata "this [valid/error] value came from cache". I believe that information belongs in a separate channel from error. For example, you could return a UserData struct which contains the data you asked for, with a separate field for its provenance - or else return a completely separate provenance value (not part of the error). On Thursday, 30 December 2021 at 00:58:04 UTC Mine GO BOOM wrote: > In practice, wrapping errors is handy for interfaces that have predefined > generic error types that will end up being logged. In code, you'd check the > error against that constant error with errors.Is(err, gateway.InvalidUser) > and handle it the same way, but when you log that error the extra messaging > will assist in debugging the problem. > > Imagine you have a gateway that provides details about users, and you have > different implementations which read from redis and one that reads from the > database. You code would call CheckUser() and the implementation would > first check redis, see it is missing, then check your database, and still > see it is missing. The redis layer would pick up on that InvalidUser error, > cache it, and return the unmodified database error. The calling code would > do a simple errors.Is(err, gateway.InvalidUser), log the error message, and > continue processing how it needs to. The next time that user is check > again, it hits redis and see that it is an invalid user. Redis returns a > wrapped InvalidUser error stating that it knows it is an invalid user at > the cache layer, maybe including the time it was stored. The calling code > checks errors.Is(err, gateway.InvalidUser), logs, and continues. > > But at that same time, the user was created in the database but the cache > wasn't invalidated. That user fails to log into the system, and reports a > bug. The developer investigates the log, sees that first database > InvalidUser log message, followed by a bunch of redis InvalidUser log > lines, intermixed with a log message saying the user was created. They > investigate the user creation code and notice it doesn't invalid the cache. > They update the code, flush the redis cache for invalid users, and resolved > a tricky caching issue that could have taken them a long time to debug > since their recreation steps may not have been to try to first login as an > invalid user before creating the user. > > As a toy example, check out: https://go.dev/play/p/7RWsj0HW1xR > > > > On Sunday, December 26, 2021 at 9:57:42 AM UTC-8 Brian Candler wrote: > >> ISTM the alternative is return a semantically-meaningful error at the >> current level of execution. >> >> e.g. if you're trying to open a config file and it fails, you return >> "Unable to open config file"; or better, include the text version of the >> underlying error: e.g. "Unable to open config file: 'foo': Permission >> denied". >> >> This is different from just returning the underlying error, and also >> different from returning a wrapped error where the caller can explicitly >> extract and match the type/value of the underlying error. >> >> Wrapping invites people to couple to the underlying error types. Having >> said that: if you don't wrap, and someone really wants to branch based on >> the underlying condition, they will match on the text content of the >> message (which is worse IMO). >> >> On Sunday, 26 December 2021 at 17:44:35 UTC fli...@flimzy.com wrote: >> >>> Yes, of course I'm quite familiar with Go 1.13's error capabilities. >>> github.com/pkg/errors is still useful when one wants to attach stack >>> traces to errors. >>> >>> But this leaves my question open: What are alternatives to wrapping >>> errors? (Other than passing around errors with no semantic meaning, as was >>> the case before wrapping was widespread) >>> >>> On Thursday, December 23, 2021 at 8:51:20 PM UTC+1 Kevin Chowski wrote: >>> >>>> Have you seen https://go.dev/blog/go1.13-errors? >>>> >>>> If one wants to use error unwrapping, these days generally it is >>>> suggested to use the functionality from the standard package "errors" (or >>>> fmt.Errorf) rather than some third-party package. That way everyone does >>>> it >>>> the same way, making third-party packages more composable by default. >>>> >>>> I personally wrap errors with fmt.Errorf and the %w verb when I want to >>>> add additional context to some error I received, and the root cause of why >>>> my function failed is *precisely* the same as the 'error' value I >>>> received. >>>> This is less often than I used to think, and it is definitely a >>>> maintenance >>>> risk when this error comes from another package: if that package changes >>>> what error type it returns, that may break some other part of my code. >>>> (that risk is perhaps one reason why folks choose not to wrap errors by >>>> default, though note that errors from Go's stdlib are less likely to >>>> change >>>> over time than some random third-party library.) >>>> >>>> In general, I prefer to wrap errors that come from a package that I own >>>> (again assuming the root cause is exactly the same AND I want to add some >>>> annotation), and prefer not to wrap errors that come from a package owned >>>> by someone else. But neither are hard rules for code I write or review. >>>> >>>> One last thought is: if you never unwrap an error, there are fewer good >>>> reasons to ever wrap them. If you want to unwrap them, then it obviously >>>> starts to make sense to wrap some :) >>>> >>>> On Thursday, December 23, 2021 at 5:59:22 AM UTC-7 fli...@flimzy.com >>>> wrote: >>>> >>>>> I was recently catching up on the latest state of the >>>>> github.com/pkg/errors package ( >>>>> https://github.com/pkg/errors/issues/245), when I noticed that Dave >>>>> Cheney said: >>>>> >>>>> > I no longer use this package, in fact I no longer wrap errors. >>>>> >>>>> I'm curious what approach Dave uses now, but unless/until he writes on >>>>> the topic, I'm also curious what other approaches people use for clean >>>>> and >>>>> cohesive error handling and reporting. >>>>> >>>>> If you don't wrap errors, how do you ensure meaningful error handling >>>>> and reporting in your application? >>>>> >>>>> Thanks, >>>>> Jonathan >>>> >>>> -- You received this message because you are subscribed to the Google Groups "golang-nuts" group. To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/eb5da6bb-b62f-43e4-8946-67413dbe1729n%40googlegroups.com.
