Azure / azure-amqp

AMQP C# library
Other
94 stars 70 forks source link

Ensure Singleton Closes Any Opened Object During Concurrent Open/Close #174

Closed paulsavides-advicent closed 3 years ago

paulsavides-advicent commented 3 years ago

See Issue = #172

Issue Overview When using the singleton class, running GetOrCreateAsync() & CloseAsync() in a particular order, the singleton will end up disposed but with Value remaining populated.

This mainly causes an issue because it is the backing for FaultTolerantAmqpObject. So, a concurrent open/close may cause an amqpobject that we would expect to be closed to remain opened in the background. I believe that is causing this issue I opened in the Azure SDK https://github.com/Azure/azure-sdk-for-net/issues/16994

Issue Reproduction The unit test in this PR contains a reproduction directly against the singleton class.

This project reproduces the issue directly using the FaultTolerantAmqpObject class https://github.com/paulsavides/ServiceBusTesting/tree/b3a5df45c67af84a816ec935d5b6bb749d915b80/ReproAmqp

When using the changes I have made in this PR, have confirmed the issues are fixed.

Issue Fix Generally just adding back a bit of code that was removed in this commit 8366a43

After actually opening the object, just add the following check:

if (this.disposed && this.TryRemove())
{
    OnSafeClose(value);
}

I briefly considered throwing an ObjectDisposedException here, however, the task result has already been set at this point so setting an exception on the task completion source would not work. Additionally, moving the IsDisposed check before the OnCreate() or SetResult would leave in a race condition so, without introducing additional locking we're stuck just returning a closed object here.

Also added a TryRemove() around both places we may call OnSafeClose(). This is just to try and ensure only one thread will actually call the remove.

Please let me know if you require any changes here, if this is the complete wrong approach or if I have completely misdiagnosed the issue.


Thank you for taking a look, Paul Savides

ghost commented 3 years ago

CLA assistant check
All CLA requirements met.

shankarsama commented 3 years ago

@xinchen10 is added to the review. #Closed

shankarsama commented 3 years ago

@yvgopal is added to the review. #Closed

shankarsama commented 3 years ago

@DorothySun216 is added to the review. #Closed