Add query notification support to SqlClient #20708
Conversation
|
Investigating NetFx build failures. |
| { | ||
| if ((null != value) && (ushort.MaxValue < value.Length)) | ||
| { | ||
| throw ADP.ArgumentOutOfRange(string.Empty, ADP.ParameterService); |
There was a problem hiding this comment.
I realize this is from the reference source, but should this be fixed to use nameof(Options) (along the lines of the other properties below) instead of ADP.ParameterService? Then the ParameterService const wouldn't need to be added.
There was a problem hiding this comment.
ParameterService has the value "Service" though, so the error message would be different from Framework.
There was a problem hiding this comment.
Options is used to set the Service Broker service name, which is why the argument is reported as "Service", I guess.
https://msdn.microsoft.com/en-us/library/system.data.sql.sqlnotificationrequest.options(v=vs.110).aspx
There was a problem hiding this comment.
Right. It looks like a bug in the desktop framework (the kind of bug that have been fixed in corefx).
There was a problem hiding this comment.
@saurabh500 Should we make the ArgumentOutOfRange exception have the actual property name here, or use the legacy "Service" name?
There was a problem hiding this comment.
FWIW we've in many places fixed the parameter name in Argument exceptions and don't consider it a breaking change at all.
| @@ -1143,7 +1143,7 @@ | |||
| "4.1.0.0": "4.1.0", | |||
| "4.1.1.0": "4.3.0", | |||
| "4.2.0.0": "4.4.0", | |||
| "4.2.1.0": "4.5.0" | |||
| "4.3.0.0": "4.5.0" | |||
There was a problem hiding this comment.
@weshaggard Should we bump to 4.2.2.0 or 4.3.0.0 here?
There was a problem hiding this comment.
@ericstj to confirm but I think we should bump the version to 4.3.0.0 given we are adding new APIs.
|
|
||
| // Constructors | ||
|
|
||
| [HostProtection(ExternalThreading = true)] |
There was a problem hiding this comment.
I believe HostProtection can be deleted. See #12081 (comment) and dotnet/coreclr#8610
| { | ||
| if (timeout < 0) | ||
| { | ||
| throw SQL.InvalidSqlDependencyTimeout("timeout"); |
| // and listen for a notification for the added commands. | ||
| if (command == null) | ||
| { | ||
| throw ADP.ArgumentNull("command"); |
| { | ||
| if (null == connectionString) | ||
| { | ||
| throw ADP.ArgumentNull("connectionString"); |
There was a problem hiding this comment.
Nit: nameof(connectionString) here and below
| { | ||
| if (_sqlDep != null) | ||
| { | ||
| _sqlDep.StartTimer(Notification); |
There was a problem hiding this comment.
Is there supposed to be a corresponding StopTimer as well?
There was a problem hiding this comment.
Seems like SqlDependency.Invalidate() handles that. There's no StopTimer() method in the Framework code.
| private void AddCommandInternal(SqlCommand cmd) | ||
| { | ||
| if (cmd != null) | ||
| { // Don't bother with BID if command null. |
There was a problem hiding this comment.
Nit: This BID comment seems unnecessary
| } | ||
| } | ||
|
|
||
| internal SqlConnectionContainerHashHelper HashHelper |
There was a problem hiding this comment.
Nit: This properties can be shorthanded HashHelper => _hashHelper
|
The Ubuntu test failure in System.Data.SqlClient.Tests.DiagnosticTest.ExecuteScalarAsyncTest is tracked by https://github.com/dotnet/corefx/issues/20441 and https://github.com/dotnet/corefx/issues/17925 .. the full details are in the console output:
@corivera @saurabh500 this test seems to fail fairly regularly in this way (only some instances are linked in the issues). Is it potentially a product issue? Could one of you please take a look? |
|
@danmosemsft That test uses a local test proxy to simulate a SQL server connection. The SQL error indicates that the proxy wasn't set up correctly, and the connection open attempt timed out. We've seen other connection issues with tests that use that test proxy code, so I suspect it's an issue with the test code itself. @saurabh500 Thoughts? |
|
Yeah, I think we need to add more logs on the server side to figure out why the server didn't come up. |
|
@dotnet-bot Test Linux x64 Tests - Debug - Ubuntu.1404.Amd64.Open please |
|
@corivera it's super confusing, but the unix test jobs all originate in a single build job, and that's the only thing that can be rerun. So @weshaggard is making this less confusing... |
|
@corivera fyi this went green. |
Addresses https://github.com/dotnet/corefx/issues/8188
Note that SqlCommand.NotificationAutoEnlist has been removed in this PR, since the underlying functionality for it does not exist. ASP.NET sets the auto enlist SqlDependency using CallContext.SetData(), but the CallContext.SetData/GetData methods are not available in .NET Core. The NotificationAutoEnlist property only checks for data set by ASP.NET, so the property has been removed to show that it's explicitly unsupported.