search

tmenier / AsyncPoco

A long-"awaited" fully asynchronous PetaPoco fork
Other
127 stars 33 forks source link

Fix: An error occurred while parallel accessing to some variables in async methods #16

Closed dolgof closed 10 years ago

dolgof commented 10 years ago

Fix: An error occurred while parallel accessing to "_sharedConnectionDepth" variable in the "OpenSharedConnectionAsync" method.

Hi! Fixed problem when calling OpenSharedConnectionAsync method from parallel tasks. _sharedConnectionDepth variable can be used without violating the logic only with a semaphore.

Here is my solution of this problem.

Thanx for the best "Awaitable " PetaPoco fork!!!

dolgof commented 10 years ago

Fix: Parallel access to _transactionDepth in BeginTransactionAsync problem

tmenier commented 10 years ago

Excellent, thank you for this contribution. I think I have a good grasp on what you did and I'd like merge it in short order, but I'm wondering if you can provide a simple unit test demonstrating the problem it solves. Or just paste a chunk of code here and I'll work it into a test. I'm interested in seeing something that fails without the fix and succeeds with it. Thanks again!

dolgof commented 10 years ago

I'll try to describe the situation that occurred in our project. We use AsyncPoco in AzureWorker. Worker core creates one StorageManager entity through which we call methods from AsyncPoco. Worker core starts the parallel tasks, receiving StorageManager as one of the parameters of an instance. Thus, all the processes used one shared instance of AsyncPoco.Database. When we create two identical tasks simultaneously (parallel attempts to load data from a database) there is an exception in 100% of cases. Step by step:

  1. The first task calls the SingleOrDefaultAsync.
  2. Then calls OpenSharedConnectionAsync() method.
  3. Then in OpenSharedConnectionAsync() calls await _sharedConnection.OpenAsync().
  4. The second task calls the SingleOrDefaultAsync.
  5. Then calls OpenSharedConnectionAsync() method in second task.
  6. _sharedConnection gets a reference to a new instance of the connection in a state of “Connecting”.
  7. Method await _sharedConnection.OpenAsync() Called in the first task completes.
  8. In the first task ExecuteReaderAsync method calls, but at this point _sharedConnection still in state of “Connecting”.
  9. Exception is: System.InvalidOperationException: ExecuteReaderAsync requires an open and available Connection. The connection's current state is “Connecting.”
tmenier commented 10 years ago

Well, I wrote a test demonstrating the issue, merged your PR, verified the fix, committed, pushed, then completely changed my mind. :)

I will offer the same reasons I gave for declining this issue. A Database object is intended to be managed similarly to a Connection object; that is, it should be created and disposed of as needed, and it is not designed to be threadsafe. The semaphore you introduced does indeed fix the problem, but I think the core issue is in your decision to use a shared instance of Database.

I would recommend that each worker thread create and dispose of Database objects as needed. This is a cheap operation; don't do what ADO.NET connection pooling is already doing for you. It also completely eliminates the need for thread synchronization.

dolgof commented 10 years ago

Many thanks for your work! And thanks for the useful advice.