Consider not throwing for completed but unsuccessful long-running operations

[pakrym]

Azure/autorest.csharp#464

From @heaths :

FWIW, Krzysztof asked that we not throw during UpdateStatus for supported status codes (e.g. 200, 202, 404) and similar calls - only when getting a value (similar to Nullable). I believe Pavel was part of this discussion as well. As a result - with oversight from Krzysztof, the Key Vault pseudo-LROs are what Krzysztof wanted: https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/keyvault/Azure.Security.KeyVault.Keys/src/DeleteKeyOperation.cs

[pakrym]

@kinelski pointed out that this would be against our current guidelines.

cc @christothes who worked on Azure/azure-sdk#2003

[heaths]

When I implemented the LROs for Key Vault (real and pseudo-LROs) back in 2019, @KrzysztofCwalina recommended we throw from Value much like Nullable<T>.Value. If recommendations have changed, please ignore my comment.

[pakrym]

No, I think you were right. The Value should throw but I don't think the UpdateStatus should.

[annelo-msft]

@tg-msft: it would be good to scope this to DPG libraries; consider pointing customers to RequestContext to control error experience.

It would be good to understand the experience in the HLC libraries as well.

[pallavit]

Do we see us doing this for exsiting HLCs? For DPG and newer light-up APIs we already have a means to achieve this behavior albeit as an advanced scenario.

Do we see us changing this to mainstream scenario however?
/cc: @annelo-msft

[annelo-msft]

@pallavit: @JoshLove-msft is driving this investigation and can make a recommendation based on his findings once available.

[pallavit]

@JoshLove-msft could you please share your current thinking here?

[JoshLove-msft]

The scope of my investigation was to determine whether or not the current shared source implementation actually aligns with the current guidelines and whether the guidelines may need updating to reflect the behavior.

The guidelines say that UpdateStatus should throw if the operation completed with a failed result. They also state that accessing the Operation Value property should rethrow the same exception for failed operations:

If the Value property is evaluated after the operation failed (HasValue is false and HasCompleted is true) throw the same exception as the one thrown when the operation failed.

The shared source implementation throws on failed results in UpdateStatus. It also throws the same exception when accessing Value.

So our official guidance in the implementation guidelines matches up with the Azure.Core shared source implementation.

The problem is that many LRO's created before the Azure.Core shared source implementation, such as Key Vault's , did the implementation themselves and do not follow the guidelines of throwing on a failed result from UpdateStatus. Changing this behavior from not throwing to throwing is a breaking change. I would recommend that these be left as is.

I do think there is room for improvement by adding a static analyzer. A simple analyzer could verify these conditions for any Operation subtype. For existing LROs that don't follow the pattern the analyzer can be disabled on a case by case basis. We could either repurpose this issue to track the static analyzer work, or open a separate issue for that and close this issue. @pallavit, any preference there?

[pallavit]

Thank you for sharing the details @JoshLove-msft

I do think there is room for improvement by adding a static analyzer. A simple analyzer could verify these conditions for any Operation subtype. For existing LROs that don't follow the pattern the analyzer can be disabled on a case by case basis. We could either repurpose this issue to track the static analyzer work, or open a separate issue for that and close this issue. @pallavit, any preference there?

As for this, I think we should open a bug in azure-sdk-tools for static analyzer. I agree with you that older SDKs would likely not be updated due to breaking changes concerns however this may be valuable for net new SDKs branded as well as other 3rd party scenarios in future. Even though LROs is a azure specific concept it may be worth evaluating it for the new toolchain as well so having a static analyzer bug for this will be useful. I would suggest tagging that issue with System.ClientModel as well so we think this scenario from that perspective as well. Let me know what you think?

[JoshLove-msft]

Works for me - filed Azure/azure-sdk-tools#7478.

[annelo-msft]

[like] Anne Loomis Thompson reacted to your message:

…

________________________________ From: JoshLove-msft ***@***.***> Sent: Wednesday, December 20, 2023 6:43:05 PM To: Azure/azure-sdk-for-net ***@***.***> Cc: Subscribed ***@***.***>; Comment ***@***.***> Subject: Re: [Azure/azure-sdk-for-net] Consider not throwing for completed but unsuccessful long-running operations (Issue #21312) Works for me - filed #21312 (comment)<#21312 (comment)>. — Reply to this email directly, view it on GitHub<#21312 (comment)> or unsubscribe<https://github.com/notifications/unsubscribe-auth/AFCIWYZGNQXJCWOI33E4X6DYKMWTXBFKMF2HI4TJMJ2XIZLTSWBKK5TBNR2WLJDUOJ2WLJDOMFWWLO3UNBZGKYLEL5YGC4TUNFRWS4DBNZ2F6YLDORUXM2LUPGBKK5TBNR2WLJDUOJ2WLJDOMFWWLLTXMF2GG2C7MFRXI2LWNF2HTAVFOZQWY5LFUVUXG43VMWSG4YLNMWVXI2DSMVQWIX3UPFYGLAVFOZQWY5LFVIYTCNZYGQ2DOOBWGCSG4YLNMWUWQYLTL5WGCYTFNSBKK5TBNR2WLKRRGIZDKOBYG44DIOFENZQW2ZNJNBQXGX3MMFRGK3FMON2WE2TFMN2F65DZOBS2YSLTON2WKQ3PNVWWK3TUUZ2G64DJMNZZJAVEOR4XAZNKOJSXA33TNF2G64TZUV3GC3DVMWTTEOJSHA4TINECUR2HS4DFUVUXG43VMWSXMYLMOVS2SOJQGAYTCMRYGU2IFJDUPFYGLJLMMFRGK3FFOZQWY5LFVIYTCNZYGQ2DOOBWGCBKI5DZOBS2K3DBMJSWZJLWMFWHKZNKGEZDENJYHA3TQNBYU52HE2LHM5SXFJTDOJSWC5DF>. You are receiving this email because you commented on the thread. Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.