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.
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.
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.
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.
@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 { } } | |||
| 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.
This method exists in AdapterUtil.SqlClient.cs It would be better to reuse it
| // 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.
Nit: remove extra dashes.
| // 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.
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.
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.
Nit: Add a new line after method ends.
There was a problem hiding this comment.
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.
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.
This should become "4.5.0.0" given you are adding new public API.
There was a problem hiding this comment.
Thanks. Updated it to 4.5.0.0
There was a problem hiding this comment.
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.
Why were these removed?
There was a problem hiding this comment.
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.
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.
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.
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.
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.
Is this used on a hot path?
| } | ||
| } | ||
| } | ||
| #endregion |
There was a problem hiding this comment.
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.
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.
Nit:
return !ADP.IsAzureSqlServerEndpoint(poolGroupConnectionOptions.DataSource);
|
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.
Nit: could this be:
if (!TryGetParsetableValue(KEY.PoolBlockingPeriod, out value))instead?
| policy = (PoolBlockingPeriod)Params[1]; | ||
| } | ||
|
|
||
| var connString = CreateConnectionString(serverName, policy); |
There was a problem hiding this comment.
Nit: all of these vars should be replaced with the actual type
| 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.
Assert.InRange(timeInSecs, 1, int.MaxValue);There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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!