Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Service Bus (potentially EH)] Check if we're always passing in the abortSignal to retry<> and the underlying operation #13052

Closed
HarshaNalluru opened this issue Jan 4, 2021 · 3 comments
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. Service Bus
Milestone

Comments

@HarshaNalluru
Copy link
Member

HarshaNalluru commented Jan 4, 2021

Check if we missed passing abortSignal in the core-amqp retry logic and add if it is required.

result = await config.operation();

@richardpark-msft @chradek @ramya-rao-a

@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Jan 4, 2021
@ramya-rao-a
Copy link
Contributor

The operation() in the retry config is a parameter less function intentionally because the retry function is agnostic to what the operation method needs. The abort signal is passed to retry() for the purpose of cancelling retries if the abort signal was fired. The same abort signal is to be used by the operation internally by the caller of retry() and should not be the responsibility of the retry() function

@ramya-rao-a ramya-rao-a added Client This issue points to a problem in the data-plane of the library. Service Bus labels Jan 4, 2021
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Jan 4, 2021
@richardpark-msft
Copy link
Member

@ramya-rao-a - I think you and @HarshaNalluru are in agreement.

I believe this issue was created to make sure we are always passing the abortSignal in to both retry and the underlying operation. If there were any cases where we hadn't, then we could end up in a situation where retry will never actually terminate since the abortSignal only applied to retry, and the underlying operation just keeps going. This is what retry<> does, so it's not a design breakage, just a "bad" usage of retry by us.

So to resolve this we just need to examine our retry calls and make sure they're following this pattern.

Alternately we could change retry() to pass this but I think the window of time to do that has passed.

@richardpark-msft richardpark-msft changed the title [Core AMQP] Check if we missed passing abortSignal in the core-amqp retry logic [Service Bus (potentially EH)] Check if we're always passing in the abortSignal to retry<> and the underlying operation Jan 28, 2021
@ramya-rao-a ramya-rao-a added this to the [2021] March milestone Jan 29, 2021
@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Feb 1, 2021

I made a pass at all the places where we are calling retry() and can confirm that the abort signal is passed to each of these calls.
When looking at whether the operation being retried is using the abort signal itself, I found the below place not meeting the bar

openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this issue Feb 25, 2021
Azure Resource Graph - Resource Changes API 2020-09-01_preview update. (Azure#13052)

* initial drop of previous version

* publishing the actual change

* pretty check update

* updating response for resource change details to return list

Co-authored-by: Alex Dubinkov <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. Service Bus
Projects
None yet
Development

No branches or pull requests

3 participants