-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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] Check abortSignal in SessionReceiver.subscribe() #11281
[service-bus] Check abortSignal in SessionReceiver.subscribe() #11281
Conversation
…er options. As part of this I can remove it from the SubscribeOptions and SessionSubscribeOptions since they already inherit from MessageHandlerOptionsBase.
…() will respect an abort signal like Receiver.subscribe() does. Adding in tests for receiver and sessionreceiver.
/azp run js - servicebus - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
|
||
if (options.abortSignal?.aborted) { | ||
throw new AbortError(StandardAbortMessage); | ||
} |
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.
This seems to be the only net change... and is very deceptively simple...
I see how the abort signal is passed to the link creation time and the one with the receiveMessages()
.
This change ensures that an already aborted signal will result in cancelling the subscribe() method.
How are we ensuring that once subscribe()
is called and then abort signal is fired that we cancel the 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.
Just to check - is this the scenario you're talking about with regards to cancellation?
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.
Discussed offline, @richardpark-msft will be logging an issue for the concern I raised above
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.
(for summary)
The abortSignal passed into subscribe, currently, only applies logically to the initialization of the subscription itself. If the subscription starts (ie, handlers are registered for receiving messages, credits on the line) the abortSignal no longer applies.
The way we have it, as is, has tripped up a few people in discussions so we want to revisit this: #11296
Check the abort signal in sessionReceiver.subscribe().
Fixes #4375
(finally)