[service-bus] pass abortSignal to link initialization and awaitableSender #15349
Conversation
chradek
requested review from
HarshaNalluru,
ramya-rao-a and
richardpark-msft
as code owners
| ); | ||
| return this._initLinkImpl(options, abortSignal); | ||
| }); | ||
| return defaultCancellableLock.acquire( |
There was a problem hiding this comment.
I want to think about this one more.
It opens up an execution path that did not exist before. It's now possible for two _initLinkImpl's to be executing at the same time if the outer lock aborts and the inner operation is still running.
This complicates the code for _initLinkImpl a bit because it now has to be careful when unwinding state that it's not conflicting with another call of itself (which wasn't a concern before).
Do we need this change?
There was a problem hiding this comment.
The abortSignal can only cause the acquire call to abort if the inner function hasn't been invoked yet. As soon as that inner function is invoked, both the timeout and abortSignal on acquire are cancelled/removed.
Does that clear up your concerns?
There was a problem hiding this comment.
It does.
Looking at the doc comment the behavior you described is pretty clear for 'task', but less clear for 'abort'.
| @@ -255,8 +280,6 @@ describe("AbortSignal", () => { | |||
| assert.equal(err.message, StandardAbortMessage); | |||
| assert.equal(err.name, "AbortError"); | |||
| } | |||
There was a problem hiding this comment.
It was a bit redundant but are the isLinkLocked calls being removed because the new cancellable lock doesn't support it?
There was a problem hiding this comment.
Yep exactly. We weren't using that API in any of our runtime code (or test code in event hubs) so I didn't implement it in the cancellable lock. We could always add it if we thought we'd gain something from it though.
| assert.fail("Should have thrown"); | ||
| } catch (err) { | ||
| assert.isTrue(sawAbortSignal, "Should have seen the abortSignal."); | ||
| assert.deepNestedInclude(err, { |
There was a problem hiding this comment.
Thanks! Someone very wise suggested it to me once 😄
Created #15417 to address this. |
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Hello @chradek! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
…Error (#15597) This PR fixes #13500. `rhea` 2.0.1 contains the fix to this specific error. We currently use `rhea` 1.x, so there's additional work in this PR to workaround the single breaking change in `rhea`, and the breaking changes in `rhea-promise`. ### rhea breaking change `rhea` contains 1 breaking change between versions 1.x and 2.x: timestamp types are now deserialized as Date objects instead of numbers. Unfortunately since this changes the way users' data might be deserialized in their service bus messages or event hubs events, we have to convert Date objects back to numbers in our client libraries until we do a major version bump. (Shorter term we can look at using rhea's default behavior behind a flag.) ### rhea-promise breaking changes Some of the `rhea-promise` APIs that accepted multiple optional positional arguments have been updated to take a single options bag parameter at the end of their method parameter list. AwaitableSender was also updated so that a timeout is no provided at instantiation. Instead, it must be provided per each `send()` call. ### core-amqp v3 Since core-amqp is being updated to depend on rhea 2.x, core-amqp dependencies will also pull in rhea 2.x transitively. To ensure that existing versions of event hubs and service bus don't break by deserializing timestamps as Date objects, core-amqp is updated to a new major version: v3. Once #15349 is merged, we can also remove `AsyncLock` completely, so I'd like to merge that PR in before releasing the changes in this PR.
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
This pull request is protected by Check Enforcer. What is Check Enforcer?Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass. Why am I getting this message?You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged. What should I do now?If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows: What if I am onboarding a new service?Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment: |
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…Error (Azure#15597) This PR fixes Azure#13500. `rhea` 2.0.1 contains the fix to this specific error. We currently use `rhea` 1.x, so there's additional work in this PR to workaround the single breaking change in `rhea`, and the breaking changes in `rhea-promise`. ### rhea breaking change `rhea` contains 1 breaking change between versions 1.x and 2.x: timestamp types are now deserialized as Date objects instead of numbers. Unfortunately since this changes the way users' data might be deserialized in their service bus messages or event hubs events, we have to convert Date objects back to numbers in our client libraries until we do a major version bump. (Shorter term we can look at using rhea's default behavior behind a flag.) ### rhea-promise breaking changes Some of the `rhea-promise` APIs that accepted multiple optional positional arguments have been updated to take a single options bag parameter at the end of their method parameter list. AwaitableSender was also updated so that a timeout is no provided at instantiation. Instead, it must be provided per each `send()` call. ### core-amqp v3 Since core-amqp is being updated to depend on rhea 2.x, core-amqp dependencies will also pull in rhea 2.x transitively. To ensure that existing versions of event hubs and service bus don't break by deserializing timestamps as Date objects, core-amqp is updated to a new major version: v3. Once Azure#15349 is merged, we can also remove `AsyncLock` completely, so I'd like to merge that PR in before releasing the changes in this PR.
…nder (Azure#15349) * [service-bus] pass abortSignal to link initialization and awaitableSender * [service-bus] add changelog entry * [core-amqp] remove AsyncLock! * [service-bus] fix test after the great merge * npm run format * fix abort on send after great merge
Fixes #15311 and #13504.
This update passed the abortSignal to the link initialization method and the awaitableSender.send() method.
It also replaces the
defaultLockfromcore-amqpwithdefaultCancellableLockwhich honors the optionalabortSignalandtimeoutInMsprops.Note: Some operations weren't supporting a timeout today, so I used the default operation timeout where it seemed to make sense to do so.