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.

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

Singleton (and FaultTolerantAmqpObject) Does not Correctly Handle Concurrent Opening & Closing #172

Closed
paulsavides opened this issue Nov 19, 2020 · 3 comments

Comments

@paulsavides
Copy link

paulsavides commented Nov 19, 2020

Reproduction Repro
Please view reproduction here >> https://github.com/paulsavides/ServiceBusTesting/blob/4cf80ba2f8e2d725b2d1923600d4aba3f26ee9b3/InterestingTests/SingletonTests.cs#L12-L38

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 doesn't technically violate the "Singleton-ness" of the class, but it introduces the same error to FaultTolerantAmqpObject. If you were to setup code like...

var creating = _faultTolerantLink.GetOrCreateAsync(Timeout);
await _faultTolerantLink.CloseAsync();

var link = await creating;

There is a possibility that you now have an active link in a 'Closed' amqpobject. The above code is, of course, a simplified example, but in more complex code its possible the area that is receiving messages & running the receive link is not aware of the area that is responsible for closing the link. So, it would be more forgivable to have the following sequence running.

I am opening this issue largely because I believe I am running into the issue in the Microsoft.Azure.ServiceBus library Azure/azure-sdk-for-net#16994


Please let me know if you require any clarification.

Thank you for taking the time to look at this issue,
Paul Savides

@paulsavides
Copy link
Author

Looking back through the history somewhat, it seems as if a check against this.disposed in the OnCreateAsync() call was removed with this commit >> 8366a43

It seems as if that would be fixed simply by adding the check back https://github.com/paulsavides/azure-amqp/commit/79bf3cfd838bd4dc35d36220067fe99047d46138

However, likely that check & close contributed to the error that commit was fixing

@paulsavides
Copy link
Author

Creating a package from this commit locally (https://github.com/paulsavides/azure-amqp/commit/c2fbd40d5bf6609ef8fb388ed8a1af8f7754f52e) I was no longer able to reproduce the issue I was seeing with the azure sdk locally.

However, that issue in general was intermittent. I will attempt to create a reproduction directly using the amqp library that can better reproduce and verify the fix.

@paulsavides
Copy link
Author

Please view this project for a reproduction using FaultTolerantAmqpObject<AmqpConnection> https://github.com/paulsavides/ServiceBusTesting/tree/b3a5df45c67af84a816ec935d5b6bb749d915b80/ReproAmqp

You can run this yourself by updating the configured KeyName, Endpoint & SharedAccessSignature in Program.cs.

I am using the Microsoft.Azure.ServiceBus utilities for creating the AmqpConnection for convenience.

Based upon the results I get from that reproduction, I am quite confident this is an issue & that my changes will fix it.

Seeing that, I will make a PR with my proposed changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants