Fix Bulk Copy Async deadlock with custom IDataReader using SqlDataReader internally.

[cheenamalhotra]

Fix for issue #755

User-scenario:

• Using 1 SqlConnection, perform Bulk Copy Async with datasource an instance of an implementation of IDataReader (internally calls SqlDataReader.ExecuteScalarAsync).

Currently, entire driver's async operations are single threaded when it comes to "parser" activity, and every Async operation is done with lock acquired from "SqlInternalConnectionTds._parserLock".

Pseudo scenario:

SqlConnection (I allow only 1 async operation to use parser) {
    SqlBulkCopy.WriteToServerAsync (parser lock acquired) {
        ReadFromRowSourceAsync (
            SimpleDataReader.enumerator => SqlCommand.ExecuteScalarAsync (I also need parser lock - Waits for lock forever!!!!!)
        )
        WriteToServerInternalRestAsync ( << parser lock is needed here)
    }
}

This PR fixes this behavior as under:

SqlConnection (I allow only 1 async operation to use parser) {
    SqlBulkCopy.WriteToServerAsync (parser lock acquired) {
        ReadFromRowSourceAsync (**release lock for reader) (
            SimpleDataReader.enumerator => SqlCommand.ExecuteScalarAsync (parser lock acquired - data read successfully)
        (**acquire lock again) )
        WriteToServerInternalRestAsync ( << parser lock is needed here)
    }
}

cc @karinazhou

[Wraith2]

The diff is a bit noisy because the IDE has fixed a lot of comments. It looks like the only change is in the bulk copy and that it saves a property releases the parser lock and then in a finally block waits, should it not re-acquire per your description?

[karinazhou]

After debugging this more, here is what I find for general scenarios where we use connection A for SqlBulkCopy instance and connection B for the data source (associated with SqlCommand). The _parserLock is associated with each connection. The lock behavior before the change is like this:

SqlBulkCopy.WriteToServerAsync() --> WriteRowSourceToServerAsync() -->  require lock on _parserLockA of connectionA
   SqlBulkCopy.ReadFromRowSourceAsync
   (
       SimpleDataReader.enumerator => SqlCommand.ExecuteScalarAsync -->  **require lock on _parserLockB of connectionB**
   )
   SqlBulkCopy.WriteToServerInternalRestAsync()  -->  need the lock on _parserLockA of connectionA

The lock behavior after the change is like this:

SqlBulkCopy.WriteToServerAsync() -->  WriteRowSourceToServerAsync()   -->  require lock on _parserLockA of connectionA
   SqlBulkCopy.ReadFromRowSourceAsync
   (
       ** release lock on _parserLockA of connectionA
       SimpleDataReader.enumerator => SqlCommand.ExecuteScalarAsync -->  **require lock on _parserLockB of connectionB**
       ** require lock on _parserLockA of connectionA
   )
   SqlBulkCopy.WriteToServerInternalRestAsync()  -->  need the lock on _parserLockA of connectionA

It looks like the original design of SqlBulkCopy doesn't expect the same connection to be used for both data source and SqlBulkCopy instance. So far, it looks OK to release the semaphore if we don't actually need it at this point to avoid the deadlock in the single connection edge case.

[cheenamalhotra]

The diff is a bit noisy because the IDE has fixed a lot of comments. It looks like the only change is in the bulk copy and that it saves a property releases the parser lock and then in a finally block waits, should it not re-acquire per your description?

It's a weird design but that's how it is.
ParserLock semaphore is of size 1, when a thread calls 'Wait' it decrements the parserlock size to 0, blocking all other 'Wait' calls till the same thread calls 'Release'. That's the definition of parserlock and that's how we can reacquire lock by calling 'Wait' again. There's no 'lock' on thread by name. This design just doesn't allow more than 1 thread to enter parser at same time.

We also discussed this internally, if another thread calls 'Wait' before we do, this thread will be on hold waiting for that thread to release parserlock before proceeding further.