Skip to content
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.

Sign up for GitHub

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

Amqp connections are not getting disposed #1771

Merged
merged 3 commits into from
Feb 4, 2021
Merged

Amqp connections are not getting disposed #1771

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0fc2600
Fixing #1770; _disposing skips the teardown
jamdavi Feb 4, 2021
02e00d0
Matching the pattern for other transports
jamdavi Feb 4, 2021
388123c
Merge branch 'master' into jamdav/issue1770
jamdavi Feb 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions iothub/device/src/Transport/Amqp/AmqpTransportHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,17 +548,12 @@ protected override void Dispose(bool disposing)
{
base.Dispose(disposing);

if (_disposed)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this a problem?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. We seem to set _disposed to true only after removing the amqp unit, so it shouldn't cause leaks, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abhipsaMisra - Are you not OOF :D Take rest!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_disposed is a protected field from the base class. That's what's throwing us all off

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when we call base.Dispose(), the _disposed is getting set to false, then we bail out prematurely.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the dispose to below the if statement to match the pattern of the other handlers.

{
return;
}

Logging.Info(this, $"{nameof(disposing)}");

if (disposing)
{
_closed = true;
AmqpUnitManager.GetInstance().RemoveAmqpUnit(_amqpUnit);
AmqpUnitManager.GetInstance()?.RemoveAmqpUnit(_amqpUnit);
_disposed = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we never use the _disposed field, we should just remove it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
Expand Down