Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Enable BeginExecuteReader methods #33660

Merged
merged 9 commits into from
Dec 20, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/System.Data.SqlClient/ref/System.Data.SqlClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,11 @@ public override void Cancel() { }
public System.IAsyncResult BeginExecuteXmlReader() { throw null; }
public System.IAsyncResult BeginExecuteXmlReader(System.AsyncCallback callback, object stateObject) { throw null; }
public System.Xml.XmlReader EndExecuteXmlReader(System.IAsyncResult asyncResult) { throw null; }
public System.IAsyncResult BeginExecuteReader() { throw null; }
Copy link
Contributor

@saurabh500 saurabh500 Nov 28, 2018

Choose a reason for hiding this comment

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

@galakt These edits should be added to System.Data.SqlClient.NetCoreApp.cs and not the System.Data.SqlClient.cs file.
These new APIs should be exposed in netcoreapp specific build. Netstandard cannot be updated.

Copy link
Author

Choose a reason for hiding this comment

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

I really confused by this:

Btw, i have moved ref changes to the NetCoreApp.cs

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @galakt
@danmosemsft My understanding is that any new APIs added even to existing types, should be moved to netcoreapp specific builds based on the conversations we had on this thread. #32245 (review)

Is this assumption correct? I guess we will need to update the baseline files to represent this.

Copy link
Author

Choose a reason for hiding this comment

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

@danmosemsft Can we finally resolve question about place for my Refs please?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we have been consistent - @ericstj?

Copy link
Author

Choose a reason for hiding this comment

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

@karelz Sorry, can you push this?

Copy link
Author

Choose a reason for hiding this comment

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

@saurabh500 @ericstj @danmosemsft Any updates on this?

Copy link
Member

Choose a reason for hiding this comment

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

SqlClient is not part of netstandard, so its APIs are not pinned by being in netstandard. You do however, need to consider support. If you add API to the reference assembly you should be sure it is supported everywhere that API can be used. So if you're exposing it in a netstandard2.0 reference assembly this means it needs to be supported in .NET 4.6.1 and later, .NETCoreApp 2.0 or later, UAP, etc (see https://docs.microsoft.com/en-us/dotnet/standard/net-standard#net-implementation-support). For UAP and NETCoreApp you can simply update the implementation assembly that is in the package, desktop is the limiting concern here since you cannot update that (and perhaps mono/Xamarin).

Copy link
Author

Choose a reason for hiding this comment

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

@saurabh500 If i understood correct, i should revert "move to NetCoreApp.cs" commit. Am i right?

Copy link
Contributor

Choose a reason for hiding this comment

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

@galakt Yes that is right.
@ericstj Thanks for the clarification.

public System.IAsyncResult BeginExecuteReader(System.AsyncCallback callback, object stateObject) { throw null; }
public System.IAsyncResult BeginExecuteReader(System.AsyncCallback callback, object stateObject, System.Data.CommandBehavior behavior) { throw null; }
public System.IAsyncResult BeginExecuteReader(System.Data.CommandBehavior behavior) { throw null; }
public System.Data.SqlClient.SqlDataReader EndExecuteReader(System.IAsyncResult asyncResult) { throw null; }
public override void Prepare() { }
public System.Data.Sql.SqlNotificationRequest Notification { get { throw null; } set { } }
public void ResetCommandTimeout() { }
Expand Down
21 changes: 18 additions & 3 deletions src/System.Data.SqlClient/src/System/Data/SqlClient/SqlCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1441,7 +1441,7 @@ override protected DbDataReader ExecuteDbDataReader(CommandBehavior behavior)
}


internal SqlDataReader EndExecuteReader(IAsyncResult asyncResult)
public SqlDataReader EndExecuteReader(IAsyncResult asyncResult)
{
Exception asyncException = ((Task)asyncResult).Exception;
if (asyncException != null)
Expand Down Expand Up @@ -1491,7 +1491,22 @@ private SqlDataReader EndExecuteReaderInternal(IAsyncResult asyncResult)
}
}

internal IAsyncResult BeginExecuteReader(CommandBehavior behavior, AsyncCallback callback, object stateObject)
public IAsyncResult BeginExecuteReader()
{
return BeginExecuteReader(null, null, CommandBehavior.Default);
}

public IAsyncResult BeginExecuteReader(AsyncCallback callback, object stateObject)
{
return BeginExecuteReader(callback, stateObject, CommandBehavior.Default);
}

public IAsyncResult BeginExecuteReader(CommandBehavior behavior)
{
return BeginExecuteReader(null, null, behavior);
}

public IAsyncResult BeginExecuteReader(AsyncCallback callback, object stateObject, CommandBehavior behavior)
{
// Reset _pendingCancel upon entry into any Execute - used to synchronize state
// between entry into Execute* API and the thread obtaining the stateObject.
Expand Down Expand Up @@ -1686,7 +1701,7 @@ protected override Task<DbDataReader> ExecuteDbDataReaderAsync(CommandBehavior b
{
RegisterForConnectionCloseNotification(ref returnedTask);

Task<SqlDataReader>.Factory.FromAsync(BeginExecuteReader, EndExecuteReader, behavior, null).ContinueWith((t) =>
Task<SqlDataReader>.Factory.FromAsync((commandBehavior, callback, stateObject) => BeginExecuteReader(callback, stateObject, commandBehavior), EndExecuteReader, behavior, null).ContinueWith((t) =>
{
registration.Dispose();
if (t.IsFaulted)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ internal bool AppDomainUnload(string appDomainKey)
private void AsynchronouslyQueryServiceBrokerQueue()
{
AsyncCallback callback = new AsyncCallback(AsyncResultCallback);
_com.BeginExecuteReader(CommandBehavior.Default, callback, null); // NO LOCK NEEDED
_com.BeginExecuteReader(callback, null, CommandBehavior.Default); // NO LOCK NEEDED
}

private void AsyncResultCallback(IAsyncResult asyncResult)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Xunit;

namespace System.Data.SqlClient.ManualTesting.Tests
{
public static class BeginExecReaderAsyncTest
{
private static string GenerateCommandText()
{
int suffix = (new Random()).Next(5000);
string companyName = "M Inc.";
string phone = "777-1111";

string commandText =
$"CREATE TABLE #Shippers{suffix}(" +
$"[ShipperID][int] NULL," +
$"[CompanyName] [nvarchar] (40) NOT NULL," +
$"[Phone] [nvarchar] (24) NULL )" +
$"INSERT INTO #Shippers{suffix}" +
$"([CompanyName] " +
$",[Phone])" +
$"VALUES " +
$"('{companyName}' " +
$",'{phone}'); " +
$"WAITFOR DELAY '0:0:3'; " +
$"select s.ShipperID, s.CompanyName, s.Phone " +
$"from #Shippers{suffix} s; ";

return commandText;
}

[CheckConnStrSetupFact]
public static void ExecuteTest()
{
using (SqlConnection connection = new SqlConnection(DataTestUtility.TcpConnStr))
{
SqlCommand command = new SqlCommand(GenerateCommandText(), connection);
connection.Open();

IAsyncResult result = command.BeginExecuteReader();
while (!result.IsCompleted)
{
System.Threading.Thread.Sleep(100);
Copy link
Contributor

@saurabh500 saurabh500 Nov 26, 2018

Choose a reason for hiding this comment

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

This test is supposed to throw InvalidOperationException when The name/value pair "Asynchronous Processing=true" was not included within the connection string defining the connection for this SqlCommand.
Doc Link

Can you check for that condition? If the connection string that is passed in doesn't have Asynchronous Processing set to true, then the test should add the parameter to the connection string.

Instead of testing BeginExecuteReader with a Sleep statement, could you test an overload which takes in a callback, which is called when the execution finishes? This way you can test EndExecuteReader in the same function and it could provide more branch coverage.
Alternatively, could you add a test which tests the callback paths?

Copy link
Contributor

Choose a reason for hiding this comment

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

And I read more documentation

https://docs.microsoft.com/en-us/dotnet/api/system.data.sqlclient.sqlconnection.connectionstring?view=netframework-4.7.2

image

Please ignore the struck out comment above.

Copy link
Author

Choose a reason for hiding this comment

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

I was confused with "Asynchronous Processing" too. I have added test with callback.

}
SqlDataReader reader = command.EndExecuteReader(result);
Assert.True(reader.HasRows, $"FAILED: Reader has no rows");
}
}

[CheckConnStrSetupFact]
public static void BeginExecuteReaderWithCallback()
{
object state = new object();
bool callbackExecutedFlag = false;

using (SqlConnection connection = new SqlConnection(DataTestUtility.TcpConnStr))
using (SqlCommand command = new SqlCommand(GenerateCommandText(), connection))
{
connection.Open();

Tuple<object, SqlCommand> stateAndCommand = new Tuple<object, SqlCommand>(state, command);

IAsyncResult result = command.BeginExecuteReader(ar =>
{
Tuple<object, SqlCommand> asyncArgs = ar.AsyncState as Tuple<object, SqlCommand>;
Assert.NotNull(asyncArgs);

SqlDataReader reader = asyncArgs.Item2.EndExecuteReader(ar);
callbackExecutedFlag = true;
Assert.True(reader.HasRows, $"FAILED: Reader has no rows");
Assert.Equal(state, asyncArgs.Item1);
}, stateAndCommand);

System.Threading.Thread.Sleep(TimeSpan.FromSeconds(10));

Assert.True(callbackExecutedFlag, $"FAILED: Callback did not executed");
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
<Compile Include="DataCommon\CheckConnStrSetupFactAttribute.cs" />
<Compile Include="SQL\AdapterTest\AdapterTest.cs" />
<Compile Include="SQL\AsyncTest\BeginExecAsyncTest.cs" />
<Compile Include="SQL\AsyncTest\BeginExecReaderAsyncTest.cs" />
<Compile Include="SQL\AsyncTest\XmlReaderAsyncTest.cs" />
<Compile Include="SQL\Common\AsyncDebugScope.cs" />
<Compile Include="SQL\Common\ConnectionPoolWrapper.cs" />
Expand Down