-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Added PoolBlockingPeriod connection property #29697
Conversation
@@ -4980,7 +4979,6 @@ | |||
"InboxOn": { | |||
"netcoreapp2.0": "4.1.1.0", | |||
"netcoreapp2.1": "4.3.0.0", | |||
"netcoreapp2.2": "4.3.0.0", |
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.
Why was this deleted ?
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 am not sure why this got deleted. These changes appeared as I ran "msbuild /t:UpdatePackageIndex" command. Looking forward to @weshaggard for more guidance on 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.
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.
Yeah, they are redundant. The actual data model should not write them but someone manually editing would. Is that what you did here @weshaggard? 2414032#diff-122916076db7087dbc454352fada61eeR2447
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.
Is that what you did here @weshaggard?
I did a couple iterations at one point I generated them with the package index update task but I may have manually changed them later for these too. At any rate @afsanehr these changes are safe but I still don't see the SqlClient version updated like I would expect. I would expect to see https://github.com/AfsanehR/corefx/blob/577778967bf657d325bd999d93dc62458e2dc0a8/pkg/Microsoft.Private.PackageBaseline/packageIndex.json#L1254 updated to the new version. Once you build the library with the new version can you again try to run the UpdatePackageIndex target on the pkgproj for it SqlClient?
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.
@weshaggard Running build -allConfigurations, I am getting this error:
D:\j\workspace\windows-TGrou---2a8f9c29\Tools\partialfacades.task.targets(64,5): error : Errors were encountered when generating facade(s). [D:\j\workspace\windows-TGrou---2a8f9c29\src\System.Data.SqlClient\src\System.Data.SqlClient.csproj]
Do you have any suggestion how to fix this?
@@ -538,6 +544,7 @@ public sealed partial class SqlConnectionStringBuilder : System.Data.Common.DbCo | |||
public override bool Remove(string keyword) { throw null; } | |||
public override bool ShouldSerialize(string keyword) { throw null; } | |||
public override bool TryGetValue(string keyword, out object value) { throw null; } | |||
public System.Data.SqlClient.PoolBlockingPeriod PoolBlockingPeriod { get { throw null; } set { } } |
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.
Nit: Indentation.
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.
done
SR.GetString(SR.AZURESQL_GermanEndpoint), | ||
SR.GetString(SR.AZURESQL_UsGovEndpoint), | ||
SR.GetString(SR.AZURESQL_ChinaEndpoint) }; | ||
private static bool IsAzureSqlServerEndpoint(string dataSource) |
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.
This method exists in AdapterUtil.SqlClient.cs It would be better to reuse it
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.
done.
// See the LICENSE file in the project root for more information. | ||
|
||
// Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
//------------------------------------------------------------------------------ |
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.
Nit: remove extra dashes.
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.
done
// The .NET Foundation licenses this file to you under the MIT license. | ||
// See the LICENSE file in the project root for more information. | ||
|
||
// Licensed under the MIT license. See LICENSE file in the project root for full license information. |
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.
Nit: Is this line in license needed? The top 3 lines seem to be available in some files that I randomly checked.
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.
Some files have this licence line and some don't. For example SortOrder.cs has it.
{ | ||
throw ADP.InvalidConnectionOptionValue(KEY.PoolBlockingPeriod, e); | ||
} | ||
} |
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.
Nit: Add a new line after method ends.
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.
Since the catch blocks are identical, this could also be:
catch (Exception e) where (e is FormatException || e is OverflowException)
{
throw ADP.InvalidConnectionOptionValue(KEY.PoolBlockingPeriod, e);
}
rather than duplicating.
@@ -604,6 +610,28 @@ internal System.Data.SqlClient.ApplicationIntent ConvertValueToApplicationIntent | |||
} | |||
// ArgumentException and other types are raised as is (no wrapping) | |||
} | |||
internal System.Data.SqlClient.PoolBlockingPeriod ConvertValueToPoolBlockingPeriod() |
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.
Nit: add new line before method begins.
cc @joperezr for version changes as well. |
src/System.Data.SqlClient/dir.props
Outdated
@@ -2,7 +2,7 @@ | |||
<Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | |||
<Import Project="..\dir.props" /> | |||
<PropertyGroup> | |||
<AssemblyVersion>4.4.1.0</AssemblyVersion> | |||
<AssemblyVersion>4.4.2.0</AssemblyVersion> |
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.
This should become "4.5.0.0" given you are adding new public API.
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.
Thanks. Updated it to 4.5.0.0
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.
Shouldn't the package Index be updated to have the 4.5 for netcoreapp2.2 now?
@@ -40,8 +41,6 @@ internal static class DEFAULT | |||
internal const bool Persist_Security_Info = false; | |||
internal const bool Pooling = true; | |||
internal const bool TrustServerCertificate = false; | |||
internal const string Type_System_Version = ""; |
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.
Why were these removed?
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.
they were mistakenly removed. fixed them.
@@ -98,6 +98,142 @@ internal static string ConvertToString(object value) | |||
throw ADP.ConvertFailed(value.GetType(), typeof(String), e); | |||
} | |||
} | |||
#region <<PoolBlockingPeriod Utility>> |
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.
Nit: space above this region line
#region <<PoolBlockingPeriod Utility>> | ||
const string PoolBlockingPeriodAutoString = "Auto"; | ||
const string PoolBlockingPeriodAlwaysBlockString = "AlwaysBlock"; | ||
const string PoolBlockingPeriodNeverBlockString = "NeverBlock"; |
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.
Rather than naming these consts like this, could you just use nameof(PoolBlockingPeriod.Auto)
, etc., at the call sites where they're used?
internal static bool IsValidPoolBlockingPeriodValue(PoolBlockingPeriod value) | ||
{ | ||
Debug.Assert(Enum.GetNames(typeof(PoolBlockingPeriod)).Length == 3, "PoolBlockingPeriod enum has changed, update needed"); | ||
return value == PoolBlockingPeriod.Auto || value == PoolBlockingPeriod.AlwaysBlock || value == PoolBlockingPeriod.NeverBlock; |
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.
What you have is fine, but this could also just be:
return (uint)value <= (uint)PoolBlockingPeriod.NeverBlock;
else | ||
{ | ||
return PoolBlockingPeriodAutoString; | ||
} |
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.
FWIW, I think this would be cleaner as a switch:
switch (value)
{
case PoolBlockingPeriod.AlwaysBlock: return nameof(PoolBlockingPeriod.AlwaysBlock);
case PoolBlockingPeriod.NeverBlock: return nameof(PoolBlockingPeriod.NeverBlock);
default: return nameof(PoolBlockingPeriod.Auto);
}
{ | ||
// We could use Enum.TryParse<PoolBlockingPeriod> here, but it accepts value combinations like | ||
// "ReadOnly, ReadWrite" which are unwelcome here | ||
// Also, Enum.TryParse is 100x slower than plain StringComparer.OrdinalIgnoreCase.Equals method. |
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.
Is this used on a hot path?
} | ||
} | ||
} | ||
#endregion |
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.
So far all of the code has been about converting to/from this enum. I'm surprised all of that code is needed.
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.
Could you elaborate more on this? There is a similar method ConvertToApplicationIntent
in here which is following the same steps . Should we create a method instead and call it inside ConvertToApplicationIntent
and ConvertToPoolBlockingPeriod
? :https://github.com/dotnet/corefx/blob/master/src/System.Data.SqlClient/src/System/Data/Common/DbConnectionStringCommon.cs#L156
else | ||
{ | ||
return true; // in Non Azure, it will be Enabled | ||
} |
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.
Nit:
return !ADP.IsAzureSqlServerEndpoint(poolGroupConnectionOptions.DataSource);
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.
done
Getting build errors in NETFX x86 Release Build CI |
{ | ||
string value; | ||
TryGetParsetableValue(KEY.PoolBlockingPeriod, out value); | ||
if (value == null) |
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.
Nit: could this be:
if (!TryGetParsetableValue(KEY.PoolBlockingPeriod, out value))
instead?
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.
done
policy = (PoolBlockingPeriod)Params[1]; | ||
} | ||
|
||
var connString = CreateConnectionString(serverName, policy); |
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.
Nit: all of these vars should be replaced with the actual type
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.
done
case PoolBlockingPeriod.Auto: | ||
{ | ||
//Disable Blocking | ||
Assert.True(timeInSecs > 0, $"Azure Endpoint with Default/Auto/Never Policy must Disable blocking."); |
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.
Assert.InRange(timeInSecs, 1, int.MaxValue);
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 was mostly looking to provide user messages in case the assertion failed for tracing the issue.
case PoolBlockingPeriod.AlwaysBlock: | ||
{ | ||
//fast failed / Enabled Blocking | ||
Assert.True(timeInSecs == 0, $"Azure Endpoint with Always Policy must Enable blocking. (Fast Failed)"); |
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.
Assert.Equal(0, timeInSecs);
@afsanehr |
@afsanehr |
@afsanehr, what's the status of this PR? |
@@ -7,4 +7,7 @@ | |||
Compat issues with assembly System.Data.SqlClient: | |||
CannotRemoveBaseTypeOrInterface : Type 'System.Data.SqlClient.SqlDataReader' does not implement interface 'System.Data.Common.IDbColumnSchemaGenerator' in the reference but it does in the implementation. | |||
MembersMustExist : Member 'System.Data.SqlClient.SqlDataReader.GetColumnSchema()' does not exist in the reference but it does exist in the implementation. | |||
TypesMustExist : Type 'System.Data.SqlClient.PoolBlockingPeriod' does not exist in the reference but it does exist in the implementation. |
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.
Update the comment above to mention why it is OK to have the PoolBlockingPeriod warning
@@ -719,6 +720,12 @@ private DbConnectionInternal CreateObject(DbConnection owningObject, DbConnectio | |||
{ | |||
throw; | |||
} | |||
#if netcoreapp | |||
if (!IsBlockingPeriodEnabled()) |
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 am OK with this approach. However an alternative could have been to call a function here CheckPoolBlockingPeriod()
;
There there could have been a partial DbConectionPool for netstandard which leaves the function empty and another DbConnectionPool.netcoreapp.cs compiled when netcoreapp DLL is built, which implements
internal void CheckPoolBlockingPeriod()
{
if (!IsBlockingPeriodEnabled())
{
throw;
}
}
@@ -8,6 +8,7 @@ | |||
|
|||
using System.Collections.Generic; | |||
using System.Data.Common; | |||
using System.Data.SqlClient; |
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 think you can get rid of this using
statement from this file.
@@ -0,0 +1,14 @@ | |||
// Licensed to the .NET Foundation under one or more agreements. |
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 think it is OK to remove .NetCoreApp.
from the filename since this file is only compile on NetCoreapp and there is no other PoolBlockingPeriod.cs
[InlineData("Azure with Never Policy must Disable Blocking", new object[] { "nonexistance.database.windows.net", PoolBlockingPeriod.NeverBlock })] | ||
public void TestAzureBlockingPeriod(string description, object[] Params) | ||
{ | ||
SqlConnection.ClearAllPools(); |
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.
This API is a bit dicey to use in Xunit tests.
We use this API in another test which causes all connection pools to clear. As a result the tests start failing intermittently. The work around was to execute manual tests Serially instead of manually. Someday I want to see these tests which call SqlConnection.ClearAllPools()
to execute as part of the RemoteExecutor app.
I am only making a note here. This is not for action for this PR.
} | ||
|
||
[Theory] | ||
[InlineData("Test policy with Always (lowercase)", new object[] { "alwaysblock" })] |
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.
This InlineData could be passed to the test above test. The two tests differ in data. The test Code is the same.
{ | ||
case PoolBlockingPeriod.Auto: | ||
case PoolBlockingPeriod.AlwaysBlock: | ||
if (e.ClientConnectionId != previousConnectionId) |
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.
Can this be Assert.Equals(...) ?
catch (SqlException e) | ||
{ | ||
// if it is the first time the exception is happening (previousConnectionId == Guid.Empty) skip the check | ||
if (previousConnectionId != Guid.Empty) |
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.
Here's my understanding:
When the SqlConnection is unable to establish a TCP connection, it sets an Empty Guid for the SqlConnection Connection Id. In your test you are setting previousConnectionId = e.ClientConnectionId
which is effectively Guid.Empty.
Is your test entering this if condition?
catch (SqlException e) | ||
{ | ||
// if it is the first time the exception is happening (previousConnectionId == Guid.Empty) skip the check | ||
if (previousConnectionId != Guid.Empty) |
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.
Similar concern as above about entering this if condition
Here's my understanding:
When the SqlConnection is unable to establish a TCP connection, it sets an Empty Guid for the SqlConnection Connection Id. In your test you are setting previousConnectionId = e.ClientConnectionId which is effectively Guid.Empty.
Is your test entering this if condition?
{ | ||
policy = PoolBlockingPeriod.NeverBlock; | ||
} | ||
string connString = CreateConnectionString(_sampleAzureEndpoint, null) + $";{_policyKeyword}={policyString}"; |
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.
Nit: This could be written as
$"{CreateConnectionString(_sampleAzureEndpoint, null)};{_policyKeyword}={policyString}"
break; | ||
case PoolBlockingPeriod.Auto: | ||
case PoolBlockingPeriod.NeverBlock: | ||
Assert.InRange(secondErrorTimeInSecs, 1, int.MaxValue); |
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.
Instead of int.MaxValue, you could check for 2*Connection Timeout. If you are not setting the connection timeout, you could use a default connection timeout of 15 sec.
|
||
public void PoolBlockingPeriodAzureTest(string connStr, PoolBlockingPeriod? policy) | ||
{ | ||
SqlConnection.ClearAllPools(); |
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 am curious, why are you clearing all pools? Is it possible to avoid this API usage. In case this is being done, so that the pool is reset for the connection string, then you could have a workaround like providing a randomly generated Application Name
in the connection string which will lead to the utilization of a new pool.
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.
The product code changes look good to me. However I have recommended a couple of modifications to the tests. Let me know what you think.
Added PoolBlockingPeriod connection property Commit migrated from dotnet/corefx@f487a4c
The src changes + tests for add PoolBlockingPeriod connection property.
@weshaggard could you please review the changes regarding the package version updates? Thanks!