Remove panics

[zezha-msft]

Fix #59 and #72.

[devigned]

This function already has an error on the return signature. Why not return an error here rather than a panic?

[zezha-msft]

Great question! We perform a sanity check here because these parameters are not supported on the service end, and there could be potential data loss if the user relied on these parameters (and we let them).

Even if we returned this as an error instead, how could the user reasonably react to it? Would they call the API again without the offending access condition?

In my understanding, errors should be actionable. In this case, it is certainly not; so it might be better to panic and let the user catch this programmer error in their testing environment.

[devigned]

I agree the error should be actionable just the same way that you would get an error from parsing something. Of course, you could retry parsing the same input, but you will still get the same error. The error is actually actionable. Most of all, it does not require special handling to ensure the application does not crash by the consumer of your API.

I find this usage of panic particularly subjective.

If you would like to indicate some level of severity with the error, then I suggest making a more informative error type. That way, you could provide some indication on the error type similar to how net.Error https://golang.org/pkg/net/#Error indicates some category of errors are temporary.

[marstr]

We keep going around and around on this. By the widely excepted convention documented in the golang wiki, panic/recover is just a way to shortcut control flow inside of your package and shouldn't be exposed through package boundaries. While calling it "sanityCheckFailed" it at least spares folks the number of hits they see when they grep panic on this library, the spirit of it doesn't change at all. Simply put, the community does not recognize any subtlety to when to return an error and when to panic: there's just a dangerous one and a safe one. Whether or not we agree with this assessment is not really relevant.

In all cases where the function already ready returns an error, at the moment is is absolutely the correct thing to use that error.

[zezha-msft]

Hi @devigned, I'm a bit confused by what you mean here:

Of course, you could retry parsing the same input, but you will still get the same error. The error is actually actionable. Most of all, it does not require special handling to ensure the application does not crash by the consumer of your API.

To clarify, the receiver SetMetadata manipulates metadata on the container, but the Storage service ignores certain container access conditions(IfUnmodifiedSince, IfMatch, and IfNoneMatch). If the user relied on these access conditions, there could be data corruption and it's very hard to recover their data when they discover this problem. In other words, we are not doing some generic parsing here; we are trying to help the user not corrupt their data. Furthermore, the same problem exists with 2 other APIs, including Delete on ContainerURL, which ignores IfMatch and IfNoneMatch. We certainly do not want the user to mistakenly delete their entire container.

If we take a step back, there are indeed other ways of helping the user with this problem. The two possibilities that I see are:

1. Return an error instead of panicking.
2. Make these 3 receivers on ContainerURL take in individual parameters instead of ContainerAccessConditions.

Would you prefer option 1 or 2? And why?

[zezha-msft]

Hi @marstr, thanks for the review. We indeed agreed to stop debating on whether panics are good/bad; instead we should be looking at our usage of panics on a case by case basis, and see if there's a way of removing it without impacting the quality/safety of the SDK. I certainly hope that you could see the progress we've made so far(there are only 13 calls to sanityCheckFailed). And with your help, I also hope to merge/publish this PR as soon as possible, to alleviate customer pain.

Here are the 13 calls to sanityCheckFailed.

1. 4 of these calls are on ContainerURL. They panic when the user relies on access conditions or parameters that are silently ignored by the Storage service. We are trying to protect the customers from data corruption/loss with the panics. We obviously do not expect the customers to write recover code for these APIs, we instead expect their code to fail immediately in the test environment when they attempt to use them.
2. 1 such call is on SharedKeyCredential's buildCanonicalizedResource. When failing to parse the query parameters, we panic instead of throwing an error because there must be a serious problem with the SDK itself if we are unable to parse the query parameters that own our code added. In other words, we can no longer proceed if we cannot trust the protocol layer.
3. Another call is on tokenCredential's constructor. We panic if the user attempts to use HTTP instead of HTTPS. This is because the user is at the risk of exposing their bearer token with HTTP. Instead of letting the call go out, we want the user to catch this in their test environment.
4. The next 2 calls are on the unmap API of the memory-mapped-file types(Unix and Windows). We've already talked about this usage, since if we cannot unmap a memory-mapped-file, we cannot proceed as there could be memory corruption.
5. The next call is in the retry policy. We panic when we are unable to rewind the body for a retry. This is a serious problem since the Body is a ReadSeeker; if we cannot rewind it for a retry, there could be data corruption if we continued. We can indeed return an error here instead of panicing, but what is the user supposed to do with this error? The last 3 panics on the list are also about seeking failures on the body.
6. The other call in the retry policy is when the response is nil while there is no error. This one is a consistency check, and basically should never happen. If we didn't check for it though, it will panic on the next line.

The above is a brief explanation for each remaining usage of panic. Each case has some nuances and took us a good amount of time to consider their validity. Please feel free to reach out if you want to discuss them.

In short, the statement that panics means unsafe code is simply not true. At least the usage number 4 proves it. If we cannot unmap a memory-mapped-file, why shouldn't we crash with a clear message? The other two options in this case are:

1. Remove the check for error
   This makes no sense, memory could be corrupted.
2. Return the error to the user
   What are they supposed to do with it? Do you really expect them to write code to detect this?

Therefore, panics are in fact a necessity in some cases.

[devigned]

@zezha-msft, let me try to clarify. You previously stated:

Even if we returned this as an error instead, how could the user reasonably react to it? Would they call the API again without the offending access condition?

I was giving an example of where an error is returned, which provides information to the user about what went wrong (actionable), but should not be retried. For example, when parsing / unmarshalling JSON into a struct the error returned is actionable, but you will receive the same result if you call unmarshal a second time. By no means should the code below panic (though it does under the covers, I believe, but is handled before it hits the package boundary).

err = json.Unmarshal(content, &structure)
if err != nil{
    fmt.Print("Error:", err)
}

In the case of SetMetadata and similar methods, if you return an error to the consumer, you'd have the same API protecting effect as the panic, but without the undesirable possibility of crashing the consumers application. Since you'd return the error after you check if etags exist, you have ensured the user will not be able to call the API (c.client.SetAccessPolicy) and possibly cause data loss.

So the short answer is that I would select option 1. Return the error. Provide adequate information in the error for the consumer to understand the reason for the error and how they would change their code to avoid the error in the future.

[zezha-msft]

Hi @devigned, thanks for the explanation! I'm ok with removing the panics in this case.

Could you please also take a look at the other cases that I've mentioned above? And see if you have any thoughts on them.

[devigned]

Thank you, @zezha-msft.

• ContainerURL
  • Return an error. I would use the same logic applied to SetMetadata and alike.
• buildCanonicalizedResource and SharedKeyCredential
  • Return an error. Since these are part of the generated code and thus should be verifiable through tests produced by the generator, I would not panic, but ensure the generated code or the generator has sufficient tests to ensure this error will not occur. Furthermore, the pipeline.PolicyFunc(func(ctx context.Context, request pipeline.Request) (pipeline.Response, error) allows for an error return, so returning an error from f.buildStringToSign will not cause much pain for upward error propagation.
• tokenCredential
  • Return an error. I would use the same logic applied to SetMetadata and alike.
• unmap
  • Panic. I'm of two minds on this one. I don't like the panic, but something is wrong at the OS level. I'm ok with bailing out on this one. @marstr, @bketelsen or @erikstmartin what do you think.
• ReadSeeker
  • Return an error explaining that the consumer must supply a ReadSeeker. I'd use the same logic applied to SetMetadata
• response == nil
  • Return an error explaining what happened and to file an issue in this repo. The function you are in already has an error return, func(ctx context.Context, request pipeline.Request) (response pipeline.Response, err error). If you'd like to have a stack for these error, consider using or building something like https://godoc.org/github.com/pkg/errors#WithStack.

[marstr]

• unmap
  • Panic. I'm of two minds on this one. I don't like the panic, but something is wrong at the OS level. I'm ok with bailing out on this one. @marstr, @bketelsen or @erikstmartin what do you think.

I agree, this seems like an acceptable place to panic.

[devigned]

I'm hoping we can remove the sanityCheckFailed panics.

[devigned]

I agree the error should be actionable just the same way that you would get an error from parsing something. Of course, you could retry parsing the same input, but you will still get the same error. The error is actually actionable. Most of all, it does not require special handling to ensure the application does not crash by the consumer of your API.

I find this usage of panic particularly subjective.

If you would like to indicate some level of severity with the error, then I suggest making a more informative error type. That way, you could provide some indication on the error type similar to how net.Error https://golang.org/pkg/net/#Error indicates some category of errors are temporary.

[zezha-msft]

@devigned @marstr I've updated the PR according to your suggestions. Please take another look and sign off if appropriate. Thanks!

[devigned]

LGTM! @zezha-msft, you are 💯