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

AutorecoveringConnection does not synchronize access to all recorded entity collections #288

Closed
Mattish opened this issue Nov 9, 2016 · 4 comments
Assignees
Milestone

Comments

@Mattish
Copy link

Mattish commented Nov 9, 2016

We've run into some Unobserved task exceptions being thrown from AutorecoveringConnection.RecoverExchanges() due to the collection being modified.

Unobserved task exception occurred 
System.AggregateException: A Task's exception(s) were not observed either by Waiting on the Task or accessing its Exception property. As a result, the unobserved exception was rethrown by the finalizer thread. ---> System.InvalidOperationException: Collection was modified; enumeration operation may not execute. 
at System.Collections.Generic.Dictionary`2.ValueCollection.Enumerator.MoveNext() 
at RabbitMQ.Client.Framing.Impl.AutorecoveringConnection.RecoverExchanges() in d:\work\RabbitMQ\projects\client\RabbitMQ.Client\src\client\impl\AutorecoveringConnection.cs:line 911 
at RabbitMQ.Client.Framing.Impl.AutorecoveringConnection.PerformAutomaticRecovery() in d:\work\RabbitMQ\projects\client\RabbitMQ.Client\src\client\impl\AutorecoveringConnection.cs:line 374 
at RabbitMQ.Client.Framing.Impl.AutorecoveringConnection.<>c__DisplayClass10.<BeginAutomaticRecovery>b__f() in d:\work\RabbitMQ\projects\client\RabbitMQ.Client\src\client\impl\AutorecoveringConnection.cs:line 352 
at System.Threading.Tasks.Task.Execute() 
--- End of inner exception stack trace --- 
---> (Inner Exception #0) System.InvalidOperationException: Collection was modified; enumeration operation may not execute. 
at System.Collections.Generic.Dictionary`2.ValueCollection.Enumerator.MoveNext() 
at RabbitMQ.Client.Framing.Impl.AutorecoveringConnection.RecoverExchanges() in d:\work\RabbitMQ\projects\client\RabbitMQ.Client\src\client\impl\AutorecoveringConnection.cs:line 911 
at RabbitMQ.Client.Framing.Impl.AutorecoveringConnection.PerformAutomaticRecovery() in d:\work\RabbitMQ\projects\client\RabbitMQ.Client\src\client\impl\AutorecoveringConnection.cs:line 374 
at RabbitMQ.Client.Framing.Impl.AutorecoveringConnection.<>c__DisplayClass10.<BeginAutomaticRecovery>b__f() in d:\work\RabbitMQ\projects\client\RabbitMQ.Client\src\client\impl\AutorecoveringConnection.cs:line 352 
at System.Threading.Tasks.Task.Execute()<--- 

In RecoverQueues() we lock on m_recordedQueues but elsewhere in the class m_recordedQueues is locked by m_recordedEntitiesLock instead. As there is no synchronization in RecoverQueues(), the Dictionary constructor uses foreach so the dictionary clone isn't protected and can throw if elsewhere manipulated.

We believe this problem will also occur for RecoverExchanges() due to no locking around the foreach but there is elsewhere in the class, although we have no seen this thrown.

We've only had 2 single cases of this happening in the past year+ of using rabbit, but it does cause the service to fall over. We are running on 3.6.5, but I can see this set of locking still exists on master

@michaelklishin
Copy link
Member

Please try 4.1.1 first.

@Mattish
Copy link
Author

Mattish commented Nov 9, 2016

The Automatic Connection and Topology recovery logic has not changed at all in the 4.1.1 AutorecoveringConnection.cs from 3.6.5. The comments regarding the lack of synchronization for those collection affects both 4.1.1 tagged and master.

@michaelklishin michaelklishin changed the title AutorecoveringConnection does not lock around all collections AutorecoveringConnection does not lock around all recorded entity collections Nov 9, 2016
@michaelklishin michaelklishin added this to the 3.6.x milestone Nov 9, 2016
@michaelklishin michaelklishin changed the title AutorecoveringConnection does not lock around all recorded entity collections AutorecoveringConnection does not synchronize access to all recorded entity collections Nov 9, 2016
@kjnilsson kjnilsson self-assigned this Nov 16, 2016
@michaelklishin
Copy link
Member

A good candidate for 3.6.7.

@michaelklishin
Copy link
Member

Closing since now stuff for 3.6.7, including #289, can be merged.

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

No branches or pull requests

3 participants