Skip to content

Conversation

@yukiwongky
Copy link
Contributor

Port fixes from Microsoft.Data.SqlClient PR#341.

Before the change, the ConnectionTime is reported as MaxValue/1000 because the _closeTimestamp was not set properly. Further, calling RetrieveStatistics multiple times causes the ConnectionTime to increase exponentially because the total connection time was SafeAdded to the previously saves _connectionTime. The _connectionTime should be overwritten instead.

When the connection is closed, the ClientConnectionId is set to empty and it's unavailable to the user. The new change allows the user to access the ClientConnectionId even after the connection is closed.

if (reconnectTask != null && !reconnectTask.IsCompleted)
// Connection closed but previously open should return the correct ClientConnectionId
DbConnectionClosedPreviouslyOpened innerConnectionClosed = (InnerConnection as DbConnectionClosedPreviouslyOpened);
if ((reconnectTask != null && !reconnectTask.IsCompleted) || null != innerConnectionClosed)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this be better as

if ((reconnectTask != null && !reconnectTask.IsCompleted) || InnerConnection is DbConnectionClosedPreviouslyOpened)
{
   ...
}

? That way we're not doing the cast unless we have to, we're not adding an unnecessary local, etc.

@stephentoub
Copy link
Member

The change this is porting added a test. Can we add the similar test here?

@stephentoub
Copy link
Member

@yukiwongky? @cheenamalhotra?

@cheenamalhotra
Copy link
Member

Hi @stephentoub I'll be creating a new PR soon to replace this one with comments addressed since Yuki is no longer working with SqlClient team.

@stephentoub
Copy link
Member

stephentoub commented Jan 15, 2020

Ah, ok, thanks for following up. I will close this then.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants