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

abort connection at sqltransaction #299

Merged
merged 3 commits into from
Nov 14, 2019

Conversation

yukiwongky
Copy link
Contributor

Fixes #291

@Wraith2
Copy link
Contributor

Wraith2 commented Nov 6, 2019

You could use a catch when clause,
catch (SqlException ex) when (ex.InnerException is Win32Exception win32ex && win32ex.NativeErrorCode == TdsEnums.SNI_WAIT_TIMEOUT) but it's just a style choice so not important if you want to keep it as is.

Test(s)?

The removal of the DoomConnectionNoReuse calls will now allow the transaction to be repooled, is that intented? If so there seem to be no callers to the method anymore so it can be removed. If aborting the connection causes it not to be re-pooled (which seem to be the suggestion from the new comment) why was this function needed originally?

@David-Engel
Copy link
Contributor

The removal of the DoomConnectionNoReuse calls will now allow the transaction to be repooled, is that intented?

It looks like dooming the connection also causes it to not be returned to the pool. CanBePooled references: (!_connectionIsDoomed && !_cannotBePooled && !_owningObject.IsAlive).

If so there seem to be no callers to the method anymore so it can be removed.

Agreed. That was added as part of the first attempted fix for 130 and should be removed here.

If aborting the connection causes it not to be re-pooled (which seem to be the suggestion from the new comment) why was this function needed originally?

Overly cautious trying to fix the issue originally since we could not repro it.

@yukiwongky
Copy link
Contributor Author

Test(s)?

I couldn't write a test for it since it's difficult to reproduce a commit timeout consistently.. The only way I could reproduce the timeout for consistently is by changing the connection timeout to 1ms in the driver. This can not be done in a client application or test because minimum timeout that can be set in the connection string is 1 second.

The removal of the DoomConnectionNoReuse calls will now allow the transaction to be repooled, is that intented?

I removed this part because by the time DoomConnectionNoReuse is called, the _innerConnection is already detached from the innerTransaction, and _innerConnection is null at that time so dooming it isn't useful.

If so there seem to be no callers to the method anymore so it can be removed.

Yes that's right. I will remove the function in the next change.

@Wraith2
Copy link
Contributor

Wraith2 commented Nov 6, 2019

This can not be done in a client application or test because minimum timeout that can be set in the connection string is 1 second.

There are a number of places in the tests that use private reflection to alter internal data to help cause specific scenarios. If the test would be repeatable with only the capacity to alter that timeout it might be worth doing the same here.

@cheenamalhotra cheenamalhotra merged commit ffd47e9 into dotnet:master Nov 14, 2019
@cheenamalhotra cheenamalhotra added this to the 1.1.0 milestone Nov 19, 2019
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.

Empty enumerables mistakenly returned during database instability
4 participants