Amqp connections are not getting disposed #1771
Conversation
jamdavi
requested review from
abhipsaMisra,
azabbasi,
barustum,
bikamani,
drwill-ms,
timtay-microsoft and
vinagesh
as code owners
| @@ -548,17 +548,12 @@ protected override void Dispose(bool disposing) | |||
| { | |||
| base.Dispose(disposing); | |||
|
|
|||
| if (_disposed) | |||
There was a problem hiding this comment.
+1. We seem to set _disposed to true only after removing the amqp unit, so it shouldn't cause leaks, right?
There was a problem hiding this comment.
@abhipsaMisra - Are you not OOF :D Take rest!
There was a problem hiding this comment.
_disposed is a protected field from the base class. That's what's throwing us all off
There was a problem hiding this comment.
when we call base.Dispose(), the _disposed is getting set to false, then we bail out prematurely.
There was a problem hiding this comment.
I moved the dispose to below the if statement to match the pattern of the other handlers.
| Logging.Info(this, $"{nameof(disposing)}"); | ||
|
|
||
| if (disposing) | ||
| { | ||
| _closed = true; | ||
| AmqpUnitManager.GetInstance().RemoveAmqpUnit(_amqpUnit); | ||
| AmqpUnitManager.GetInstance()?.RemoveAmqpUnit(_amqpUnit); | ||
| _disposed = true; |
There was a problem hiding this comment.
if we never use the _disposed field, we should just remove it.
There was a problem hiding this comment.
nvm. Talked to James offline and he explained it to me.
Based on our conversation, we can still keep the if(_disposed) check if we move it above the base.Dispose() call.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Can we add anything to catch this before customers? E2E test? Not this PR but after this. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
We could add some monitoring to long haul. To do this in an integration test we'd need to mark the I'll put a PR out for that to see what everyone thinks. |
Awesome! We can see how the test looks but we should definitely add some monitoring to long haul as we did not catch it when we released this time and this seems to be something that we should cover as a basic functionality. |
Fixes #1770 by reverting regression introduced in this change.