[service-bus] Allow initLink() and closeLink() to lock, preventing reentrancy/concurrency issues #10807
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…he open/close state of the link into LinkEntity.
The detach() calls have been simplified dramatically. Logically this appears okay - all init()/close() calls will run serially now (thanks to the lock) so it's perfectly safe to allow them to "coallesce". The net result will still be an initialized connection.
…etached - Remove unneeded checkAborted method - we can just let the normal initLink function handle it.
* Fix isOpen() to return false when the internal link hasn't yet been defined * Strip out <a:address> from the log prefix - it's already encoded into the entity name.
…rtSignal handling (initLink will handle it all).
richardpark-msft
requested review from
chradek,
HarshaNalluru and
qiaozha
as code owners
ghost
added
the
ramya-rao-a
reviewed
Aug 24, 2020
…at's used for messages.
…ack2-further-cleanup-1
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ramya-rao-a
approved these changes
Aug 26, 2020
…live environment. Moving my utility function into 'misc' which is more obviously just unit-test compatible functions.
Co-authored-by: Ramya Rao <[email protected]>
Co-authored-by: Ramya Rao <[email protected]>
Co-authored-by: Ramya Rao <[email protected]>
Co-authored-by: Ramya Rao <[email protected]>
Co-authored-by: Ramya Rao <[email protected]>
…hub.com/richardpark-msft/azure-sdk-for-js into richardpark-sb-track2-further-cleanup-1
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
… and is standardized (and more generic).
…ng into a common init() function that handles locking ,etc...
…ing the sending portion (not init) were being "eaten" because the callback passed to the Promise is async.
…sed. - Also, update some log messages to be a bit more accurate.
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
I had inadvertantly created an eternal call stack that would never end by causing negotiateClaim to cause continual calls to initLink and closeLink. This isn't even a recovery scenario as all the scenarios would eventually either fail by exhausting retries or because the link does eventually open.
In addition I added in some code and tests for a dead lock scenario that I observed. Basically we could end up with this call stack:
linkEntity.initLink()
<enter lock>
linkEntity.negotiateClaim()
connectionContext.readyToOpenLink()
refreshConnection() occurs
calls linkEntity.closeLink()
<attempts to enter lock and blocks>
The fix to this was two-fold:
1. Check the ready state of the connection before even entering into lock for initLink.
2. Within initLink, before we call negotiateClaim, do a status check to ensure that the connection is "ready". If it is not then go ahead and throw.
…ulate conditions with the connection.
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ramya-rao-a
reviewed
Aug 27, 2020
ramya-rao-a
reviewed
Aug 28, 2020
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ramya-rao-a
reviewed
Aug 28, 2020
…hub.com/richardpark-msft/azure-sdk-for-js; branch 'master' of https://github.com/Azure/azure-sdk-for-js into pr/richardpark-msft/10807
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…in the process of reopening.
…hub.com/richardpark-msft/azure-sdk-for-js into richardpark-sb-track2-further-cleanup-1
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…not in a restarting state (and more unit tests for it).
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
initLink and closeLink had some basic coordination so they wouldn't stomp over each other but they never coordinated via locks which made it so the state of the object would be different than you expected.
This commit makes it so both closeLink and openLink depend on the same lock, which allows you to make the modifications to the link state obey ordering. This simplifies some parts of the code that tried to avoid opening and closing simultaneously.
As part of this the lock token that was being used in other parts of the code has been subsumed into LinkEntity instead so some modification to the detached/init methods of child classes changed.
Also, removed some code in ManagementLink that has become redundant with our unified initLink() code (abort signal handling and some link state checking).
Fixes #10656