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
[core-rest-pipeline] Bug Fix - Retry logic to honor abort signal #20781
Conversation
… harshan/issue/20778
@@ -113,7 +113,7 @@ export function retryPolicy( | |||
strategyLogger.info( | |||
`Retry ${retryCount}: Retry strategy ${strategy.name} retries after ${retryAfterInMs}` | |||
); | |||
await delay(retryAfterInMs); | |||
await delay(retryAfterInMs, undefined, { abortSignal: request.abortSignal }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It always feels cumbersome to have to pass in undefined in the middle. Can we make value
(or valueToResolve) part of the options bag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I put everything in the options bag instead?
I feel the "value" is more important and it is better to not put it in the options bag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my impression is that only the first parameter is used most of time. But no strong feeling. This is not public.
* @param value - The value to be resolved with after a timeout of t milliseconds. | ||
* @returns Resolved promise | ||
* @param options - The options for delay - currently abort options | ||
* @param abortSignal - The abortSignal associated with containing operation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does tsdoc/jsdoc work in this way for nested properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*/ | ||
export function delay<T>(t: number, value?: T): Promise<T | void> { | ||
return new Promise((resolve) => setTimeout(() => resolve(value), t)); | ||
export function delay<T>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t know if the delay function is the best place to put this code.
However, I understand why it would make sense to put it in the delay code.
So I think it’s ok!
Do we have other examples of the delay function handling the abort signal?
I wonder if this is going to be a pattern moving forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this delay
function with abortSignal support be in core-rest-pipelines or perhaps in another core package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not written this from scratch, this was from core-http
.
And we also have a similar(identical) method in core-amqp
and service-bus
also relies on it I think.
Basically, it's already prior art for a few years now and I'm not introducing any new patterns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this delay function with abortSignal support be in core-rest-pipelines or perhaps in another core package?
@sadasant It is now added in core-rest-pipeline
.
I would prefer core-util
, but it is not a dependency of core-rest-pipeline
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, I like the design! A lot! Just food for thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohh I understand now! Alright
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks for the fix!
… harshan/issue/20778
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let’s go!
I was surprised to see we never ported this from core-http -- apparently this was hanging out on our backlog this whole time: #16118 😅 |
Oh damn, thanks for pointing out, @xirzec! It was so old that I didn't even realize I logged that issue. 😂 |
…re#20781) * throttlingRetryPolicy should honor abort signal * puppeteer for browser tests * changelog * Update sdk/core/core-rest-pipeline/CHANGELOG.md * update jsdoc * changelog * assert.isRejected * lint:fix * lock file
…re#20781) * throttlingRetryPolicy should honor abort signal * puppeteer for browser tests * changelog * Update sdk/core/core-rest-pipeline/CHANGELOG.md * update jsdoc * changelog * assert.isRejected * lint:fix * lock file
[Hub Generated] Publish private branch 'datafactory/mitra/updatescripttoscriptlines' (Azure#20781) * [AutoSync] ee38db2819 Update documentation with scriptLines * [AutoSync] ee38db2819 Update documentation with scriptLines Co-authored-by: swagger-automation <swagger@microsoft.com>
Packages impacted by this PR
@azure/core-rest-pipeline
Issues associated with this PR
Fixes #20778
Describe the problem that is addressed by this PR
Retry logic in core-rest-pipeline doesn't honor abort signal.
Found while investigating #20766
Added a new delay method that takes in abort options, Using the new delay method in the place of the older delay method in the retry logic.
Are there test cases added in this PR? (If not, why?)
Yes
Checklists