[azservicebus] Fixing bug where cancelling link creation could leak resources #18479
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
|
/azp run go - azservicebus |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run go - azservicebus |
|
(condition was inverted, fixing) |
|
/azp run go - azservicebus |
|
Azure Pipelines successfully started running 1 pipeline(s). |
1. When the amqpwrap code is returning a concrete object as part of the return value and it's being "converted" into an interface we need to make sure we return the correct 'nil'. 2. The bigger bug: when amqpLinks was creating links it would assign the result of the function to it's internal field directly in the call. Since <interface>nil isn't the same as <concrete-type>nil it would look like there was a Receiver to close, even when there wasn't.
|
/azp run go - azservicebus |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run go - azservicebus |
|
Azure Pipelines successfully started running 1 pipeline(s). |
jhendrixMSFT
approved these changes
Jun 27, 2022
|
@richardpark-msft thx, for quick fix |
richardpark-msft
pushed a commit
to richardpark-msft/azure-sdk-for-go
that referenced
this pull request
Jun 30, 2022
…f3cb This go-amqp update lets us cancel links directly, rather than having to do our workaround with the goroutines. It also swaps some of the core types from using variadic config to using struct based config instead. In addition, Updating with some changes from Azure#18479: - Fixing bug where we were assigning non-functional entities to amqpLinks's fields - Fixing a bug where I was doing the (interface, nil) instead of (nil, nil) for stuff coming back from amqpwrap!
richardpark-msft
added a commit
that referenced
this pull request
Jun 30, 2022
…a7e7511a069acc4f3cb (#18503) This go-amqp update lets us cancel links directly, rather than having to do our workaround with the goroutines. It also swaps some of the core types from using variadic config to using struct based config instead. In addition, Updating with some changes from #18479: - Fixing bug where we were assigning non-functional entities to amqpLinks's fields - Fixing a bug where I was doing the (interface, nil) instead of (nil, nil) for stuff coming back from amqpwrap!
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.
Attempting to shoe-horn the draining of the channel into the same goroutine doesn't work - it leads to a potential race condition or can lead to a blocking condition identified by @franklinkim in #18477.
The simplest fix is just to treat the original goroutine as a cancellable function call and then handle the return value logic inside of the main func instead. This ensures we always drain the 'done' channel, even if we decide to cancel early.
NOTE, this also unearthed a bug where we were sometimes passing (amqp., nil) instead of (, nil) in the amqp wrappers. This was fixed, along with the spot that was unintentionally using it even when there was an error.