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

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

Merged
merged 2 commits into from
Nov 4, 2020

Conversation

cheenamalhotra
Copy link
Member

@cheenamalhotra cheenamalhotra commented Oct 30, 2020

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
Copy link
Contributor

Wraith2 commented Oct 31, 2020

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
Copy link
Member

karinazhou commented Oct 31, 2020

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
Copy link
Member Author

cheenamalhotra commented Oct 31, 2020

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.

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

Successfully merging this pull request may close these issues.

4 participants