aws / aws-sdk-net

The official AWS SDK for .NET. For more information on the AWS SDK for .NET, see our web site:
http://aws.amazon.com/sdkfornet/
Apache License 2.0
2.07k stars 862 forks source link

AmazonSQSClient.DeleteQueueAsync should not throw exception when deleted non-existent queues #3018

Closed Coder3333 closed 1 year ago

Coder3333 commented 1 year ago

Describe the feature

AmazonSQSClient.DeleteQueueAsync throws the following exception if you try to use it to delete a queue that was previously deleted. I am requesting that this scenario would no longer throw an exception. I do not see any benefit in DeleteQueueAsync throwing an exception for a queue that does not exist, since the goal of the api is to get rid of a queue.

Amazon.SQS.AmazonSQSException: The specified queue does not exist for this wsdl version. ---> Amazon.Runtime.Internal.HttpErrorResponseException: Exception of type 'Amazon.Runtime.Internal.HttpErrorResponseException' was thrown. at Amazon.Runtime.HttpWebRequestMessage.GetResponseAsync(CancellationToken cancellationToken) at Amazon.Runtime.Internal.HttpHandler1.InvokeAsync[T](IExecutionContext executionContext) at Amazon.Runtime.Internal.Unmarshaller.InvokeAsync[T](IExecutionContext executionContext) at Amazon.SQS.Internal.ValidationResponseHandler.InvokeAsync[T](IExecutionContext executionContext) at Amazon.Runtime.Internal.ErrorHandler.InvokeAsync[T](IExecutionContext executionContext) --- End of inner exception stack trace --- at Amazon.Runtime.Internal.HttpErrorResponseExceptionHandler.HandleExceptionStream(IRequestContext requestContext, IWebResponseData httpErrorResponse, HttpErrorResponseException exception, Stream responseStream) at Amazon.Runtime.Internal.HttpErrorResponseExceptionHandler.HandleExceptionAsync(IExecutionContext executionContext, HttpErrorResponseException exception) at Amazon.Runtime.Internal.ExceptionHandler1.HandleAsync(IExecutionContext executionContext, Exception exception) at Amazon.Runtime.Internal.ErrorHandler.ProcessExceptionAsync(IExecutionContext executionContext, Exception exception) at Amazon.Runtime.Internal.ErrorHandler.InvokeAsync[T](IExecutionContext executionContext) at Amazon.Runtime.Internal.CallbackHandler.InvokeAsync[T](IExecutionContext executionContext) at Amazon.Runtime.Internal.Signer.InvokeAsync[T](IExecutionContext executionContext) at Amazon.Runtime.Internal.EndpointDiscoveryHandler.InvokeAsync[T](IExecutionContext executionContext) at Amazon.Runtime.Internal.EndpointDiscoveryHandler.InvokeAsync[T](IExecutionContext executionContext) at Amazon.Runtime.Internal.CredentialsRetriever.InvokeAsync[T](IExecutionContext executionContext) at Amazon.Runtime.Internal.RetryHandler.InvokeAsync[T](IExecutionContext executionContext) at Amazon.Runtime.Internal.RetryHandler.InvokeAsync[T](IExecutionContext executionContext) at Amazon.Runtime.Internal.CallbackHandler.InvokeAsync[T](IExecutionContext executionContext) at Amazon.Runtime.Internal.CallbackHandler.InvokeAsync[T](IExecutionContext executionContext) at Amazon.Runtime.Internal.ErrorCallbackHandler.InvokeAsync[T](IExecutionContext executionContext) at Amazon.Runtime.Internal.MetricsHandler.InvokeAsync[T](IExecutionContext executionContext)

Use Case

The reason this is such an issue is that AmazonSQSClient.ListQueuesAsync will return queues that were already deleted, which means that my application has no way to know which queues actually exist prior to calling DeleteQueueAsync. To make things more complicated, I cannot simply wrap the call to DeleteQueueAsync in a catch block and blindly eat all exceptions, as I would want to react differently if the api failed for a networking issue.

Proposed Solution

If AmazonSQSClient.DeleteQueueAsync cannot find the queue being requested, do not throw an exception. Update DeleteQueueResponse to have a flag indicating whether the requested queue was found.

Other Information

I already requested that AmazonSQSClient.ListQueuesAsync not return deleted queues, but was told that was not going to happen. I believe simply having AmazonSQSClient.DeleteQueueAsync not throw an exception for this case is a better solution to the problem.

https://github.com/aws/aws-sdk-net/discussions/2451

Acknowledgements

AWS .NET SDK and/or Package version used

AWSSDK.SQS 3.7.100.30

Targeted .NET Platform

.Net 6

Operating System and version

Windows 10

ashishdhingra commented 1 year ago

@Coder3333 Thanks for submitting the feature request. Per service API documentation at DeleteQueue, QueueDoesNotExist error is thrown if this operation is invoked for a non-existent queue.

The workaround in discussion https://github.com/aws/aws-sdk-net/discussions/2451 was proposed due to limitation of the service API, not in the SDK.

For logic in AWS SDK(s) which is auto-generated (as mentioned in discussion https://github.com/aws/aws-sdk-net/discussions/2451), per design it would return any exception returned by the service to the calling code. The response object DeleteQueueResponse shape is derived from the service model shared by the service team. Hence, we could not simply add a new flag since it would be overwritten when the class is auto-generated by service models in next build/push.

Instead of wrapping the calls to DeleteQueueAsync() in try{}catch{} block, you may implement a utility method which can return the status of the call (refer similar utility method AmazonS3Util.DoesS3BucketExistV2Async() for S3).

Thanks, Ashish

Coder3333 commented 1 year ago

@ashishdhingra , automatically waiting 60 seconds before any call to List, just to make sure any previous calls to Delete have completed is not reasonable at all. Also, it does not make a lot of sense for me to manage a helper function that ignores specific exceptions when I do not know what the full list of ignorable exceptions is, nor do I know when those change. It would make a lot more sense for AWS to manage that helper function, since they intimately know the return codes from the service. The exception doesn't even provide an enumeration that I can inspect to determine the reason.

ashishdhingra commented 1 year ago

@ashishdhingra , automatically waiting 60 seconds before any call to List, just to make sure any previous calls to Delete have completed is not reasonable at all. Also, it does not make a lot of sense for me to manage a helper function that ignores specific exceptions when I do not know what the full list of ignorable exceptions is, nor do I know when those change. It would make a lot more sense for AWS to manage that helper function, since they intimately know the return codes from the service. The exception doesn't even provide an enumeration that I can inspect to determine the reason.

@Coder3333 Thanks for your reply. I would review this with team and post any updates here.

normj commented 1 year ago

Hi @Coder3333, I agree with you that have to do control flow by exceptions is not ideal but utility method from the SDK method that essentially buries the exception isn't practical for us. The SDK doesn't know the intent and can't tell the difference if this is a not found because of an eventual consistent issue between the list call or the queue name is just invalid. Then we have a consistence issue with the SDK itself because there are 1000+ delete methods across the SDK, should they all of a utility method? But we don't have any meta data from our service definitions that we use to generate the service client to inform our generator if and how to generate the utility method.

github-actions[bot] commented 1 year ago

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see. If you need more assistance, please either tag a team member or open a new issue that references this one. If you wish to keep having a conversation with other community members under this issue feel free to do so.