azblob.NewSharedKeyCredential should not panic

[jamesjoshuahill]

When we create a new Shared Key Credential, and pass in a account key which is an invalid base 64 string, the library panics. Normally in Golang a panic means something went unexpectedly wrong, which is not the case in this function call, and the error can be easily returned.

For us as consumers of the package we have to recover the panic in order to prevent our program from exiting unexpectedly. Also, the error passed to panic does not indicate that it was caused by an invalid account key.

Would you accept a PR?

@jamesjoshuahill and @tinygrasshopper

[JeffreyRichter]

You bring up a very controversial subject that is worth some discussion. First, I agree with you that the panic/error should be clear about what went wrong and I'm happy to fix that and also happy to take a PR for this. Now, whether to panic or return error is the controversial part. I have spent many months contemplating this exact issue. You say that panic means that something unexpectedly went wrong. I find this"definition" to be too vague to be actionable. There are lots of things in Go itself that cause panics. For example, division of an integer by zero, attempting to write to a closed channel, accessing an invalid memory address, accessing an index that is out of range, or numerous reflection methods (if you go to the documentation for Go's reflect package and search for the word 'panic', you'll get 82 occurrences!). For the exact example you mention, I think passing an invalid base 64 string for account key is "unexpected" and so I think that panicking is the best choice here because you will be notified of a bug in your code immediately and you'll fix the bug. As opposed to having the SDK return an error because the error is not actionable in YOUR code. I believe that if I returned this error to you that your code can't execute more other code that corrects the base64 string so you can try calling the SDK method again. And, if you think you will try using bad base64 strings, for some reason that I can't imagine, you can verify them yourself before calling the SDK method and handle the error and avoid the panic. Or, you can recover from the panic -- your choice. As many people have pointed out all over the internet, error handling in Go is a pain. I completely agree with these sentiments because I struggle with it a lot when designing this sdk and I see that the Go team themselves are rather inconsistent. I certainly hate checking if an err is not nil and then trying to figure out what to do next. Also, methods that return 2 or more values are not composable. That is, if a method returns only 1 value, your code can say ".Something else()" on the same line of code making your code more succinct. I really like composability and Go's error handling pattern greatly reduces your ability to use it. Panicing instead of returning an error brings back composability. So, these are the things I think about when designing whether to panic or return error. If I think it's likely a particular error can occur AND I think customer code should be able to deal with this error at runtime because it is most likely not a bug in your code, then I make the method return an error. All SDK methods that do I/O operations return an error, for example. Since I/O errors are fairly common with distributed systems and you can retry them or somethings, you actually expect the error. For example, getting a blob not found when trying to delete a blob is frequently considered an ok operation and your code can just continue running. If I think the error is due to a programmer bug which cannot be solved at runtime but should be solved by fixing code, then I panic so you can find your bug quickly and fix it. And, in addition, by panicking, I've offerer you composability.

…

-- Jeffrey Richter (watch Architecting Distributed Cloud Apps<https://aka.ms/RichterCloudApps> on YouTube<https://aka.ms/RichterCloudApps> or edX<https://aka.ms/edx-devops200_9x-about>)

________________________________ From: Josh Hill <notifications@github.com> Sent: Tuesday, April 24, 2018 1:59 AM To: Azure/azure-storage-blob-go Cc: Subscribed Subject: [Azure/azure-storage-blob-go] azblob.NewSharedKeyCredential should not panic (#42) When we create a new Shared Key Credential, and pass in a account key which is an invalid base 64 string, the library panics. Normally in Golang a panic means something went unexpectedly wrong, which is not the case in this function call, and the error can be easily returned. For us as consumers of the package we have to recover the panic in order to prevent our program from exiting unexpectedly. Also, the error passed to panic does not indicate that it was caused by an invalid account key. Would you accept a PR? @jamesjoshuahill<https://github.com/jamesjoshuahill> and @tinygrasshopper<https://github.com/tinygrasshopper> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<#42>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ACK0v7uV3FfkpLoLyQkOY2bzur8fbn55ks5trulegaJpZM4ThRRY>.

[tombuildsstuff]

@JeffreyRichter to give a counter point here, we load clients on demand using an access key, if the credentials are invalid (such as when the access tokens have been cycled) it’s perfectly safe to pull the latest access key and try again. This policy would be a blocker for us adopting this SDK, since it means users would have unexpected crashes (which isn’t acceptable for our use-case) - as such I’d like to +1 this request to use errors rather than panics; as has been done in the Go SDK.

Regarding method chaining - this is less prevalent in Go than other languages (such as C# or Java), since most complex methods can return errors - as such I believe the downsides to this approach outweigh the benefits of panicking here

Thanks!

cc @joshgav

[JeffreyRichter]

Hold on, I think we’re conflating 2 different things here. Your original post said you were passing in an invalid base 64 string. If you have a no-longer-valid account key, then the string is still a valid base64 string and creating a SharedKeyCredential will NOT panic. In fact, if you have a no-longer-valid key, then the I/O request will be signed with it and the request will be sent to the Azure Storage service successfully. When the service gets it, it will simply return a valid HTTP response with a status code of 403 (forbidden). Again, a panic does not happen in this case. Instead, the API you’re calling will return a non-nil error and you CAN look at this error, create a new pipeline with a new SharedKeyToken (with a new account key) and call your desired API again. So, from what you are describing, I do not see any potential blocker for you adopting the library. And also, if a panic did occur, then you can use Go’s defer/recover mechanism to recover from this and do what you want. So, you really should be fine. Have you tried it?

…

-- Jeffrey Richter (watch Architecting Distributed Cloud Apps<https://aka.ms/RichterCloudApps> on YouTube<https://aka.ms/RichterCloudApps> or edX<https://aka.ms/edx-devops200_9x-about>) From: Tom Harvey <notifications@github.com> Sent: Monday, July 2, 2018 9:27 AM To: Azure/azure-storage-blob-go <azure-storage-blob-go@noreply.github.com> Cc: Jeffrey Richter <jeffrichter@live.com>; Mention <mention@noreply.github.com> Subject: Re: [Azure/azure-storage-blob-go] azblob.NewSharedKeyCredential should not panic (#42) @JeffreyRichter<https://github.com/JeffreyRichter> to give a counter point here, we load clients on demand using an access key, if the credentials are invalid (such as when the access tokens have been cycled) it’s perfectly safe to pull the latest access key and try again. This policy would be a blocker for us adopting this SDK, since it means users would have unexpected crashes (which isn’t acceptable for our use-case) - as such I’d like to +1 this request to use errors rather than panics; as has been done in the Go SDK. Regarding method chaining - this is less prevalent in Go than other languages (such as C# or Java), since most complex methods can return errors - as such I believe the downsides to this approach outweigh the benefits of panicking here Thanks! cc @joshgav<https://github.com/joshgav> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#42 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ACK0v5UEQknHNE12tJ3QjTFqxAIK-b2Dks5uCknjgaJpZM4ThRRY>.

[devigned]

So, from what you are describing, I do not see any potential blocker for you adopting the library.
And also, if a panic did occur, then you can use Go’s defer/recover mechanism to recover from this and do what you want. So, you really should be fine.

I don't agree that one should use recover in this situation. If you are taking input from a user, you can't control if they fat finger something. Using a panic in this type of situation is excessive and unlike any other library I've used.

Recover does not provide the same experience as the common error return. The return is close the problem where recover is distant. Recover requires handling in a defer, where error can be handled in normal control flow. It's apples and oranges.