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

User message for non-support of Sys.Transactions in SqlClient #21765

Closed
saurabh500 opened this issue May 17, 2017 · 6 comments
Closed

User message for non-support of Sys.Transactions in SqlClient #21765

saurabh500 opened this issue May 17, 2017 · 6 comments
Assignees
Milestone

Comments

@saurabh500
Copy link
Contributor

I am opening this issue to discuss the right customer messaging for absence of Sys.Transactions support in SqlClient.
Refer comment https://github.com/dotnet/corefx/issues/19708#issuecomment-301959886 and the following comment.

In case of SqlClient operations enclosed in a TransactionScope, the operations would not be run in a single transaction.
However this would not throw an exception to the customer. We need a mechanism to make sure that customers understand this behavior.

@saurabh500
Copy link
Contributor Author

saurabh500 commented May 17, 2017

One of the proposal I make, is to Check in SqlClient operations (SqlConnection.Open() , SqlCommand.Execute*() ) to see if there is an active transaction scope and show an error to the customer that they are trying to use TransactionScope with SqlClient which is not supported and the implicit Transactional behavior is not guaranteed.
The user can set an AppConfig flag to turn off this error message to opt-in for the absent functionality.

This proposal would make the customer opt-in to use SqlClient operations in TransactionScope, knowing that the implicit transactions enlisting is not supported.

The AppConfig switch will be removed once we add the Sys.Transactions support in SqlClient.
Due to absence of support of Distributed Transactions in Sys.Transactions, there would still be gaps in functionality with the Desktop.

@saurabh500
Copy link
Contributor Author

saurabh500 commented May 17, 2017

cc @karelz @FransBouma @NickCraver @divega @danmosemsft

@saurabh500 saurabh500 self-assigned this May 17, 2017
@divega
Copy link
Contributor

divega commented May 17, 2017

One of the proposal I make, is to Check in SqlClient operations (SqlConnection.Open() , SqlCommand.Execute*() )

AFAIR, auto-enlistment only happens during Open/OpenAsync, so it should be enough to do it there.

We can also override DbConnection.EnlistTransaction for the purpose of throwing with the same message.

FWIW, we are already doing something similar in EF Core layer, and the plan is to remove it when we have actual support.

@saurabh500
Copy link
Contributor Author

We can also override DbConnection.EnlistTransaction for the purpose of throwing with the same message.

Makes sense.
Since Enlist default value is true for Sql connection string, it makes enlisting in Ambient transactions a default behavior.

Essentially we are down to throwing an exception for

  1. SqlConnection.Open/OpenAsync if it is inside a TransactionScope
  2. DbConnection.EnlistTransaction override in SqlConnection

@divega For dotnet/corefx#1 do you think that it makes sense to have an opt-in behavior with an AppCompat switch to disable the exception so that the customer uses the feature after knowing that its risks?

@divega
Copy link
Contributor

divega commented May 17, 2017

@saurabh500 I have a concern with having a temporary flag that just bypasses the exception: assume once we start supporting Sys.Tx again, we stop paying attention to this flag and connections are auto-enlisted again. This will have significant side effects on the behavior of the app, but can easily go undetected. It seems much better to tell users to disable auto-enlistment in the connection string to avoid the exception. Then they can choose to opt back in (or not) once we have the support added.

@saurabh500
Copy link
Contributor Author

saurabh500 commented May 17, 2017

@divega Thanks for the feedback. I had missed out the fact that I can re-introduce the Enlist keyword and take a decision based on the keyword along with a current transaction in Sys.Transactions .

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.0.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants