-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Add async overloads to SslOverTdsStream #27743
Conversation
@@ -48,6 +50,51 @@ public void FinishHandshake() | |||
/// <param name="count">Byte count</param> | |||
/// <returns>Bytes read</returns> | |||
public override int Read(byte[] buffer, int offset, int count) | |||
{ | |||
return ReadInternal(buffer, offset, count, CancellationToken.None, false).GetAwaiter().GetResult(); |
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.
@stephentoub I have tried to reuse code and use the same method for Read and ReadAsync. Looking forward to any feedback here.
From what I understand there should be no allocations due to this approach, but looking forward to your review.
uint status = TdsEnums.SNI_SUCCESS; | ||
try | ||
{ | ||
await stream.WriteAsync(_data, 0, _length, CancellationToken.None); |
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: please add .ConfigureAwait(false)
to the end of this
} | ||
}); | ||
|
||
packet.WriteToStreamAsync(_stream, cb); |
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 could end up invoking callback
while holding the lock. Is that ok?
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 looking into the need for the locks in Async Scenarios and running some tests to validate the removal of these locks only in case of async reads and writes.
SqlClient on Windows uses locks for some sync cases, however no locks are used for async scenarios and that parity can be applied to the Managed Networking stack of SqlClient as well.
If my validations don't show any regressions, then I will update the code with locks removed from async reads and writes.
SNIPacket newPacket = packet; | ||
|
||
_writeTaskFactory.StartNew(() => | ||
SNIAsyncCallback cb = callback ?? _sendCallback; |
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.
Do these changes also need to be ported to NpHandle? If so, we might as well make a base class for both handles, since they only differ in the underlying streams they open/use.
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 will change NpHandle as well.
@@ -249,7 +249,7 @@ public void ReadFromStreamAsync(Stream stream, SNIAsyncCallback callback, bool i | |||
options |= TaskContinuationOptions.LongRunning; | |||
} | |||
|
|||
stream.ReadAsync(_data, 0, _capacity).ContinueWith(t => | |||
stream.ReadAsync(_data, 0, _capacity, CancellationToken.None).ContinueWith(t => |
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.
Use await?
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.
@corivera I will send out another PR changing the readAsync piece and making it use async await.
currentOffset += currentCount; | ||
} | ||
} | ||
|
||
|
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.
Extra space
@@ -47,7 +49,50 @@ public void FinishHandshake() | |||
/// <param name="offset">Offset</param> | |||
/// <param name="count">Byte count</param> | |||
/// <returns>Bytes read</returns> | |||
public override int Read(byte[] buffer, int offset, int count) | |||
public override int Read(byte[] buffer, int offset, int count) => | |||
ReadInternal(buffer, offset, count, CancellationToken.None, false).GetAwaiter().GetResult(); |
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.
It looks like previously this would have called the synchronous Read methods on the underlying stream, but now it's invoking the asynchronous ones and then blocking on the result. Why? That's generally not a good direction to go.
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.
Ah, nevermind, I see you're switching in the implementation on using either the sync or async methods. Ok.
Could you name the argument to ReadInternal so that it's clear what the bool means, e.g. async: false
instead of just false
?
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.
OK.
@@ -195,6 +280,7 @@ public override long Seek(long offset, SeekOrigin origin) | |||
throw new NotSupportedException(); | |||
} | |||
|
|||
|
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.
Extra space
else | ||
{ | ||
readBytes += _stream.Read(packetData, readBytes, TdsEnums.HEADER_LEN - readBytes); | ||
} |
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: you could simplify this slightly, e.g.
readBytes += async ?
await _stream.ReadAsync(packetData, readBytes, TdsEnums.HEADER_LEN - readBytes, token).ConfigureAwait(false) :
_stream.Read(packetData, readBytes, TdsEnums.HEADER_LEN - readBytes);
readBytes += _stream.Read(packetData, readBytes, TdsEnums.HEADER_LEN - readBytes); | ||
if (async) | ||
{ | ||
readBytes += await _stream.ReadAsync(packetData, readBytes, TdsEnums.HEADER_LEN - readBytes, token).ConfigureAwait(false); |
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 library targets netstandard2.0 rather than netcoreapp2.1, right? If it targeted the latter, it'd be nice to use the new Memory-based overloads, but I don't think it does.
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.
Yes, SqlClient targets netstandard2.0. I would like to change it to target netcoreapp
I will open an issue to track the work. Then SqlClient can benefit more from the Memory Based overloads.
/// <param name="count"></param> | ||
/// <param name="token"></param> | ||
/// <param name="async"></param> | ||
/// <returns></returns> |
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: you can delete all of these empty comments
In the commit that I have pushed , I have added an additional change which removes locks in SNITcpHandle.cs during async reads and writes. |
@stephentoub is this good with you? |
SNILoadHandle.SingletonInstance.LastError = new SNIError(provider, SNICommon.InternalExceptionError, e); | ||
status = TdsEnums.SNI_ERROR; | ||
} | ||
callback(this, status); |
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's expected to happen if the callback throws an exception? Is that possible?
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 callback is not expected to throw any exceptions and handles all the exception using a
try { } catch (Exception e) {}
block and handling all the exception itself.
}); | ||
|
||
packet.WriteToStreamAsync(_stream, cb, SNIProviders.TCP_PROV); | ||
} |
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.
Same question I had before... this is now potentially invoking cb
while holding the lock... is that ok?
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 could remove the locks from async reads and writes. As a result none of the operations are now holding a lock. I had another change for removal of locks during async reads and writes being tested. I clubbed the change with this PR. Please see comment at #27743 (comment)
} | ||
catch (IOException ioe) | ||
{ | ||
return ReportErrorAndReleasePacket(packet, ioe); |
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: these catch blocks could be consolidated, e.g.
catch (Exception e) when (e is ObjectDisposedException || e is SocketException || e is IOException)
{
return ReportErrorAndReleasePacket(packet, 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.
Sounds good.
@dotnet-bot Test NETFX x86 Release Build please |
NETFX x86 Release Build has been timing out
The changes in this PR only impact Linux and macOS. |
* Add async overloads for SslOverTdsStream for async operations * Remove Unused Code * Expression bodied syntax * Address CR comments and remove locks in async operations * Remove Read Locks * Remove async logs from NpHandle * Remove extra space * Consolidate the exceptions Commit migrated from dotnet/corefx@68cfdde
Added
ReadAsync
andWriteAsync
overloads to SslOverTdsStream in SqlClient so that the async apis over encrypted connection are full async instead of calling theRead
andWrite
methods of SslOverTdsStream.I also modified the SNITcpPacket to call into the Stream's WriteAsync APIs.
This impacts SSL connections to Sql server.
Tests done: