Please don't panic on DecodeString

[devigned]

I was surprised to see all of the implementations of NewSharedKeyCredential panic when decoding the account key.

azure-storage-blob-go/2018-03-28/azblob/zc_credential_shared_key.go

Line 23 in 197d1c0

panic(err)

I would expect the error to be returned, rather than a panic.

@JeffreyRichter, there are over 100 panic calls in this library. Is this intended? I haven't found an instance of a panic call which wouldn't be better handled by returning an error. Thoughts?

[JeffreyRichter]

Please see the discussion here.

[tombuildsstuff]

@JeffreyRichter is there a rough timeline to remove the panic's from this provider and instead return errors from methods? We can send a PR for this if needed - as this issue is unfortunately blocking our adoption of this SDK

[arschles]

I'd love to see a timeline too.

I'd most like to see errors returned because the compiler and standard static analysis tools will make sure I handle the errors.

[tombuildsstuff]

👋 I've noticed #63 which fixes the specific issue the API call raised above (and have reviewed it / commented on the potential crashes as I saw them).

Cloning the repository locally I notice there's 477 panic's across 66 files in this repository (and that's just grepping for panic( / excluding the less obvious items like nil-pointers; presumably the scope of this issue is to remove all of those panic's (e.g. at a minimum the panic() calls) - rather than just the one in this PR - or should I open a separate issue to track that?

Thanks!

[zezha-msft]

Hi @tombuildsstuff, thanks for the review! I'll make the changes soon.

There's actually 186 panics in the 2018-03-28 version. I do not think that we should blindly remove all of them, since there are many valid scenarios to use panic to help the user detect their programming errors.

Is there other panic that you think is not valid?

[tombuildsstuff]

@zezha-msft

Hi @tombuildsstuff, thanks for the review! I'll make the changes soon.

No problem - thanks for working through them :)

Is there other panic that you think is not valid?

All of them, within all SDK/API versions (at least, for the user-facing parts of the code, within tests is up to you).

I do not think that we should blindly remove all of them, since there are many valid scenarios to use panic to help the user detect their programming errors.

Libraries in Go use the convention of returning errors rather than panicking so that the callers can decide how to respond to errors (which also makes it obvious which functions could return errors based on the return types - which becomes part of the libraries Public API). By contrast - Libraries which panic make it extremely challenging to handle errors correctly, since it means we need to know about the internal workings of each method and /when/ a panic could occur - rather than it being explicit and being caught at compile time.

FWIW I can find this relevant blog post from the Go site on the topic, which states: It's worth stressing that whatever the design, it's critical that the program check the errors however they are exposed. The discussion here is not about how to avoid checking errors, it's about using the language to handle errors with grace..

As such I believe all of the uses of panic need to be removed from (at least the user-facing) this library and replaced with errors - else unfortunately I don't see how we could migrate from the current Azure SDK for Go Storage SDK to this one.

Thanks!

[tombuildsstuff]

@JeffreyRichter is there any update here? I'm wondering if we should be forking this SDK to remove the panic's (described above) - or if we should hold off if that work's in progress? Thanks!

[JeffreyRichter]

Yes, I have an update. I have been really, really struggling with this. There is truly a lack of official guidance on the use of errors versus panics. For example, the Go Language Specification only explains how these mechanisms work and gives no guidance for when to favor one over the other. The official Go blog has a few articles published in 2010 and 2011 that discuss panics. Only the 2010 blog post offers any kind of guidance at all about panics; it says:

The convention in the Go libraries is that even when a package uses panic internally, its external API still presents explicit error return values.

All other articles, videos, and books that I could find related to panic/error guidance seem to have extrapolated guidance from this one sentence and have taken it to an extreme. The reality is that Go packages panic all the time. For example, if you pass nil for an interface, slice, or func parameter to a method defined in a Go package, the Go runtime panics on that package's behalf when it tries to use the nil value. If you look at the package's source code, you just don't see an explicit call to panic. Also, many, many Go packages explicitly call panic: look at the reflect package documentation and search for the word panic; it appears more than 80 times! Look at the documentation for Go's bytes package and search for panic; again you'll see several methods that panic. This is just the tip of the iceberg, there are many Go packages that panic. Amazon's S3 Go SDK (which is very similar to this repo's blob SDK) also panics.

Due to the lack of official panic guidance, I took it upon myself to write an article that talks about Embracing Panics in Go Packages. I've not published the article because I know it will be very controversial in the Go community. But, I am making it available here so that you and others can read it just to hear an alternate perspective.

About 2 weeks ago, I went to Google in Seattle and met with Brad Fitzpatrick (a member of the Go team). I shared my article with him and he said he'd share it with other Go team members; I haven't gotten any feedback yet. However, as Brad & I spoke and researched many existing packages, we very quickly realized that the current messaging in the community of when to use panics and what packages were really doing were not lining up. This proved that the official guidance is lacking and that the Go team should improve it.

At the same meeting, Brad and I also took a stab at coming up with some official error/panic guidance that could be shared with the Go community at large. Here is what we came up with (so far):

• A function should always document what it panics unless the panic is due to an implicit Go runtime panic such as nil interface/function/*T.
• It is OK to panic (and not return an error) if a sanity check fails. (A sanity check is something you really, really don’t expect to fail in a specific scenario).
• If you want to avoid returning an error to allow greater composability of code, then it is OK to panic. This needs more guidance in general as to when to determine the severity making error better/worse than panic and also some guidance about versioning: what if you don’t return an error today but decide to return an error tomorrow?
• Validate all inputs being passed to a standard library function such that it won’t panic – I feel that this is untenable in real life. That is, no one will actually do this.
• If the code would panic implicitly anyway, it is OK to validate an argument and explicitly panic. For example, instead of letting the runtime panic due to nil or array access out of bounds, the function can check for nil/bounds and explicitly panic giving a better error message.

The list above is from my notes and should NOT be taken as official guidance as of today. If this guidance were to become official, then this repo's blob SDK is currently following best panic/error guidance. Except that we are missing some documentation for methods that do panic; we will correct this.

I'm hoping to continue my involvement with Google's Go team to drive the creation of better official error/panic guidance. With their new error proposals for Go, now is a great time to get all of this sorted out and improved for the whole Go community.

[zezha-msft]

This is fixed in 0.3.0.