-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Recover connection state counting after external close #9792
Conversation
_openedCount++; | ||
} | ||
|
||
_openedCount++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting that the tests pass, but this won't work for connection resiliency. Count would be incremented on every retry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AndriySvyryd I added a test using connection resiliency and fixed a test bug, but can't get product code to to fail. Probably going to need some help writing a test that demonstrates this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the issue in test code: TestRelationalTransaction.ClearTransaction()
calls _testConnection.Close()
for some reason, hiding the product bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AndriySvyryd I simplified TestRelationalTransaction and TestSqlServerConnection so that they don't have any logic other than causing failures...still can't reproduce the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, that was unrelated then. The code you added to TestRelationalCommandBuilderFactory
hides a bug.
I've also added tests for the other places where an ExecutionStrategy
is used: see origin\MoreResiliencyTests
ce4bf0d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New bits up with query fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add tests for and fix connection resiliency
7f1c8dc
to
abd3931
Compare
Issue #9524 The issue here was the reference count was not kept correctly if the connection was closed externally when it was not expected to be. The fix is to keep the count correct in this case.
abd3931
to
ef95d9f
Compare
Issue #9524
The issue here was the reference count was not kept correctly if the connection was closed externally when it was not expected to be. The fix is to keep the count correct in this case.