Ssl Stream Async Write #23715
Ssl Stream Async Write #23715
Conversation
|
All Outerloop tests should be run prior to merging. |
|
The general approach looks pretty good to me. Curious to hear @stephentoub's thoughts. I like how you put the cancellationToken on the adapter, that fits in nicely. I'll add a few specific comments... |
| // No "artificial" timeouts implemented so far, InnerStream controls timeout. | ||
| lazyResult.InternalWaitForCompletion(); | ||
| private async Task WaitForWriteIOSlot<T>(T writeAdapter, Task lockTask, byte[] buffer, int offset, int count) | ||
| where T : ISslWriteAdapter |
There was a problem hiding this comment.
Just to be safe, this should probably be "where T : ISslWriteAdapter, struct"
There was a problem hiding this comment.
changed, (needs to be struct, ISslWriteAdapter but same diff :P )
| internal interface ISslWriteAdapter | ||
| { | ||
| Task LockAsync(); | ||
| void Release(); |
There was a problem hiding this comment.
Not sure having Release on the interface is useful. It's not different b/w sync and async.
There was a problem hiding this comment.
That is true. The only reason i did it was because i am not 100% sure if it should be sync or not. It currently boots any pending handshake if you are writing off to another thread.
Interestingly the previous implementation has a comment also asking if that is the right thing to do ....
There was a problem hiding this comment.
It does feel a bit awkward there but it is also the only place the write side needs the sslstream ref and it kinda goes with the lock unlock logic... but I am on the fence so if you think it should go I am cool with that, your choice
There was a problem hiding this comment.
Removed the Release, it's not just release anyway its a finishwrite so makes sense to be removed
|
@davidsh Definitely I am not keen on merging any SslStream code without a full outerloop :) |
|
|
||
| namespace System.Net.Security | ||
| { | ||
| internal interface ISslWriteAdapter |
There was a problem hiding this comment.
I wonder if the interface and impls should be nested inside SslStreamInternal. That's presumably the only place they will ever be used, right?
If we needed the adapter to be able to call some private method on SslStreamInternal, then it would be nice to have them nested. As it stands it doesn't matter much, but it might in the future.
There was a problem hiding this comment.
I am okay with that if it is what you would prefer, however it means eventually 4 structs + 2 interfaces (read/write side async/sync) which is a decent chunk at the bottom of the class. Another option is I could partial class StreamInternal so they are still in another file but have private access?
There was a problem hiding this comment.
moved it inside SslStreamInternal using a partial class and made the methods private. It also exposed the fact that my SingleChunk and MultipleChunk methods were internal when they could/should be private so I changed that as well.
| internal bool CheckEnqueueWrite(AsyncProtocolRequest asyncRequest) | ||
| internal Task CheckEnqueueWriteAsync() | ||
| { | ||
| //Clear previous request. |
|
|
||
| namespace System.Net.Security | ||
| { | ||
| //This contains adapters to allow a single code path for sync/async logic |
There was a problem hiding this comment.
Nit: space after // (same goes for any other occurrences)
| public Task WriteAsync(byte[] buffer, int offset, int count) | ||
| { | ||
| _sslState.InnerStream.Write(buffer, offset, count); | ||
| return Task.CompletedTask; |
There was a problem hiding this comment.
If all call sites of this just await the resulting task, then this is fine. If this method were exposed otherwise, though, it should really be:
try
{
_sslState.InnerStream.Write(buffer, offset, count);
return Task.CompletedTask;
}
catch (Exception e) { return Task.FromException(e); }in order to prevent exceptions from synchronously escaping the Task-returning method and instead encapsulate them into the returned task.
Given that this is only meant to be used from synchronous callers and exceptions will ultimately emerge synchronously from the calls, it's probably fine.
| ValidateParameters(buffer, offset, count); | ||
|
|
||
| SslWriteAsync writeAdapter = new SslWriteAsync(_sslState, cancellationToken); | ||
|
|
There was a problem hiding this comment.
Nit: there's a blank line here but not in the corresponding Write code.
| { | ||
| throw new ArgumentException(SR.Format(SR.net_io_async_result, asyncResult.GetType().FullName), nameof(asyncResult)); | ||
| throw new NotSupportedException(SR.Format(SR.net_io_invalidnestedcall, "WriteAsync", "write")); |
There was a problem hiding this comment.
Nit: "WriteAsync" => nameof(WriteAsync)
| if (t.IsCompleted) | ||
| { | ||
| _nestedWrite = 0; | ||
| return t; |
There was a problem hiding this comment.
The else case calls ExitWriteAsync, which calls _sslState.FinishWrite(); if there's an exception and also checks the exception type and ensures it's an IOException. Why isn't that necessary on this path? Seems like the IsCompleted check should be changed to IsCompletedSuccessfully.
There was a problem hiding this comment.
Changed to completedsucessfully
| } | ||
| } | ||
|
|
||
| private Task WriteSingleChunk<T>(T writeAdapter, byte[] buffer, int offset, int count) | ||
| where T : struct, ISslWriteAdapter |
There was a problem hiding this comment.
Nit: often "T" is used to represent data elements. I think it'd be nice to be more descripive here, e.g. TWriteAdapter. Same for any other places this pattern is used.
| return CompleteAsync(t, rentedBuffer); | ||
| } | ||
|
|
||
| async Task WaitForWriteIOSlot<T2>(T2 wAdapter, Task lockTask, byte[] buff, int off, int c) |
There was a problem hiding this comment.
Why does this need its own generic type parameter... doesn't it inherit the one from the containing method?
There was a problem hiding this comment.
i didn't realise that worked, changed.
| { | ||
| throw new InvalidOperationException(SR.Format(SR.net_io_invalidendcall, "EndWrite")); | ||
| //Single write | ||
| Task t = WriteSingleChunk(writeAdapter, buffer, offset, count); |
There was a problem hiding this comment.
It looks like WriteSingleChunk can throw synchronously. Don't we need to handle that here?
There was a problem hiding this comment.
Yeah, I think the best way is to wrap that throw in inside the WriteSingleChunk into a
Task.FromException and return that, then it will be picked up by the normal flow without more try catches?
There was a problem hiding this comment.
Fixed by returning the exception inside a task
|
Broken commit I am fixing it. |
|
@dotnet-bot Test Outerloop Windows x64 Debug Build |
|
Did we ever get perf data here? It would be nice to see if this gives a win at all (or at least doesn't regress). |
|
Yeah I was planning on it but... day job has been busy for a week and needed azure to up my core count. Will work on getting it done this week. The inproc version of your test shows some disturbing results with the current code. E.g. as many rps on a 20 core box as a 4 core laptop. So I need to make a clean setup and get proper numbers. |
|
By current I mean before this PR. |
|
I seem to be having issues with my azure account. Here are the numbers I have managed to run locally on my machine (Windows 10 4 core and 256 connections)
Managed to sort my Azure account and using 2 8 core Windows 2016 machines I get
/cc @geoffkizer |
|
@geoffkizer any thoughts on this? |
|
Results look great to me. I think we addressed all the PR feedback right? |
| // Re-handshake status is not supported. | ||
| ArrayPool<byte>.Shared.Return(rentedBuffer); | ||
| ProtocolToken message = new ProtocolToken(null, status); | ||
| return Task.FromException(new IOException(SR.net_io_encrypt, message.GetException())); |
There was a problem hiding this comment.
Do we need to do anything to "unlock" in a failure case like this, or has that already been taken care of implicitly? It's a bit jarring to a "LockAsync" call without a corresponding release, but maybe that's just a minor naming issue if it's actually handled under the covers.
There was a problem hiding this comment.
Yeah it's related to the way the lock is done APM style under the hood in SslState (something I hope to change in the near future). The call to FinishWrite releases any lock and that is caught in finally blocks higher up. It's just an interlocked based lock so we could call it a second time for clarity if you wish.
There was a problem hiding this comment.
If it's correct, we can leave it as-is for now, since your change doesn't appear to be making it worse. It'd be good to re-evaluate this in the near future as more work is done, as you say.
|
@dotnet-bot Test Tizen armel Debug Build |
|
Other failures System.Net.Security.Tests.NegotiateStreamTest on Ubuntu.1604.Amd64.Open-Release-x64 Only platform these tests are enabled for, oversight? System.Net.Sockets.Tests on suse.422.amd64.Open Only platform this test is enabled for, oversight? System.Drawing.Common.Tests - Debian.90.Amd64.Open - Catastrophic failure |
|
Outerloop failures not related to this change? (See above) |
|
DualModeConnectionlessReceiveFrom is a flaky test, see #23535 |
|
Does that mean it's passed, worth running again? |
|
@dotnet-bot Test Outerloop UWP CoreCLR x64 Debug Build |
|
Was it Einstein that said "Insanity: doing the same thing over and over again and expecting different results"? ;) |
|
@davidsh Is this drawing failure the problem I have seen you working on? |
I am not working on this CI failure in System.Drawing.Common.Tests: @stephentoub Who is working on this CI failure?
|
@mellinoe has been trying to track down the cause of this drawing seg fault. |
|
Okay cool, as long as it's nothing we need to worry about in this PR. If that is the case, is it ready to merge? |
|
@dotnet-bot Test Outerloop UWP CoreCLR x64 Debug Build |
|
@dotnet-bot dotnet-bot Linked Test Outerloop Linux x64 Release Build |
|
@JeremyKuhne, the UWP leg keeps hanging in the FileSystem tests. Known issue? Presumably that's unrelated to this PR, but we should be sure. |
I'm not aware of an issue. Do we make hang dumps in the CI? |
|
@dotnet-bot Test Outerloop UWP CoreCLR x64 Debug Build testing 1 more time before launching an issue for it |
|
Same test is hanging in other unrelated PR's Issue raised, #24217 |
|
Thanks, @Drawaes |
|
Cool :) I will take a look at the rest ;) |
* Microsoft.ServiceModel.Syndication skeleton project * Adding the existing classes of SyndicationFeed from .net fx * Added the needed references to get the code to compile * Changed some namespaces * Fixed errors when reading feeds and replaced some buffers * Cleaning code for PR * Deleted some unused files * Added the posibility that the user creates his own date parser when reading a feed from outside, also as part of our default date parser, if it can't parse the date it will just assign a default date to avoid the crash * Added correct testnames and Copyright * Initial changes to add custom parsers * Added more delegates as parsers * Added custom parser delegates * Save changes * Initial SyndicationFeed * Removed the dependence of the old SR class * Cleaned the code, deleted Diagnostics, fixed throwing resources * Cleaned code from most of the unnecesary comments * Formatted the code with CodeFormatter Tool * Moved the call of itemParser to be called with each item * Test using custom parsers * Fixed image issue where image tag was using original title and url, for RSS formatting Image issue fixed and added some tests * Test clase jorge * Save changes with Jorge cass * Initial Jorge * saving changes * Save changes * Fixed image and items issue * Fixed disjoint items in atom * Run codeFormatter * Adding parsing to optional elements int the spec added unittesting * Added Icon parsing to Atom10 feedFormatter. Added unit test * Adding Async Writing RssFormater writes new optional spec elements * Added Icon writing on Arom Writer * Fixed some warnings * Improved custom parsing for RSS * Added custom parsing for Atom feed formatter, added test * added nameof() to all exceptions when needed. * Adding Extension Methods for XmlReader * Fixing code for PR * Fixing code for PR * Added check for skip days to allow only accepted days. * Improved flexibility for Default dateparser * Add wrong dates example * Fixing code review * Fixed warnings on some unawaited methods. * Added async extension methods for XmlReader * Add XmlWriterWrapper method extensions * Changed ReadCategoryFromAsync to return a SyndicationCategory * Fixed sync method exposed GetReader. * Edited XmlReaderWrapper, moved methods to extension methods. * Removed unnecesary variables from Wrappers. * Fixed bug from ServiceModel.Syndication * Make BigInteger ctor opt path for == int.MinValue reachable. The BigInteger(ReadOnlySpan<byte> value) ctor contains an optimised path for value being larger than 4 bytes and the result being equal to int.MinValue. However this path is inside a test that precludes that value, so can never be reached. Restructure so that the path is reachable. * Fix ServiceModel.SyndicationFeed project dependencies - Change M.SM.SyndicationFeed to be a .NET Standard 2.0 library - Change tests to use official .NET Core 2.0 release from preview * Add btrfs and other missing file system types (dotnet#24102) * Uppercase * Add more filesystems from stat.c. Change to stat names where possible * Add some more local friendly names * Add some more remote file types from stat.c * Add entry to switch present in enum * Build errors * Remove Fixed entries that should be RAM * comment * Change case of 0X to 0x * Move cramfs to Fixed * ConcurrentQueue 128-byte cache line (dotnet#22724) ConcurrentQueue 128-byte cache line * Add warning by default in SGEN (dotnet#24054) * Output warning by default if run the tool directly without /quiet parameter. * add quiet parameter in the command. * fix parameter error. * Update the warning. * Add the target to copy the serializer to publish folder. (dotnet#24096) * Wrap cert callback leaked exceptions in WinHttpHandler (dotnet#24107) Similar to the fix done for CurlHandler (dotnet#21938), fix WinHttpHandler so that leaked exceptions from the user-provided certificate callback are properly wrapped. The ManagedHandler still needs to be fixed since it is not wrapping properly. Contributes to #21904 * Ensure ProcessModule for main executable is first in modules list (dotnet#24106) * Disable NegotiateStreamTest fixture for distros without working Kerberos (dotnet#24098) Disable NegotiateStreamTest fixture entirely because setup-kdc.sh is broken on some distros * Expose and add tests for Guid.ctor(Span)/TryWriteBytes/TryFormat * Address remaining PR feedback * #24112 Replaced documentation summary of TryPop with text similar to TryDequeue. (dotnet#24113) * Wrap exceptions from ManagedHandler's server validation callback (dotnet#24111) To match other handlers' behaviors. * Fix libgdiplus function loading on OSX. * Prevent ServiceControllerTests from dispose when already disposed (dotnet#24042) * Disable ServiceProcessTest that has been failing on CI and official builds * Prevent dispose from been called when already disposed * Add logging to repro if RemoveService is been called twice on the same ServiceController * Data-annotations length fix (dotnet#24101) * Updated in MinLengthAttribute and MaxLengthAttribute to support ICollection<T> * Added tests * Fixed typo * Trying to address two failing checks: - Linux x64 Release Build - UWP CoreCLR x64 Debug Build * Implemented changes requested in review - Extracted Count checking to an external helper to obey DRY - Removed dependency of ICollection<T> and changed to simple reflection Count property lookup * Added requested tests * Added catch for MissingMetadataException. * Extracted code from try-catch. * Added comment as requested. * Typo correction * Remove System.Drawing.Common from the netfx compat package (temporary). * Updating corefx to use new roslyn compiler (dotnet#24076) * Updating corefx to use new roslyn compiler * Updating to new version of the compiler and use the switch so tests pass on desktop * Fix System.Reflection.Metadata tests * Stop running Math.Clamp tests on UAP as API is not there (dotnet#24125) * Stop running Math.Clamp tests on UAP as API is not there * Disable only for AOT * Fix instantiating PrincipleContext with Domain type (dotnet#24122) * Fix instantiating PrincipleContext with Domain type * Enhancement * Disable Test_Write_Metric_eventListener on UWP. (dotnet#24127) * Revert "Remove System.Drawing.Common from the netfx compat package (temporary)." This reverts commit f6b0fbd. * Update BuildTools, CoreClr, CoreFx, CoreSetup, Standard to prerelease-02019-01, preview1-25718-02, preview1-25718-03, preview1-25717-02, preview1-25718-01, respectively (dotnet#24075) * Fixed compile warning/error on FreeBSD (dotnet#24141) * Marking {ReadOnly}Span as readonly structs (dotnet#23908) * Marking {ReadOnly}Span as readonly structs, fixing issue #23809 * Adding readonly attributes to reference assemblies. * Using "readonly ref" keyword instead of attributes. * Adding a LangVersion 7.2 property * System.Drawing: Throw ArgumentNullException on Unix as well (dotnet#24140) * Throw ArgumentNullException when stream is null * Throw ArgumentNullException when stream is null * Remove invalid NameResolution tests (dotnet#24147) The Dns_GetHostEntryAsync_* are fundamentially invalid because it's possible for the broadcast address, 255.255.255.255, to have an DNS mapping via manually modifying the hosts file. This was actually happening on Mac systems as well as an virtual environment running on top of that (i.e. Windows on Parallels). Ref: https://github.com/dotnet/corefx/issues/23992#issuecomment-330250642 Contributes to #23992 * Bump system.drawing.common.testdata to 1.0.6 * Update BuildTools to prerelease-02019-02 (dotnet#24146) * Fix path to test data * Fix whitespace * Add GraphicsTests based on Mono's test suite. * Consolidate more code in the "System.Drawing" namespace. * Remove all remaining Win32 codepaths from the mono codebase. All of this code now implicitly assumes that it will be run on a Unix platform. * Consolidate the rest of the gdipFunctions.cs file into Gdip.cs and GdipNative.Unix.cs * Consolidate the GraphicsUnit and ImageType enumerations -- they were duplicated. * Remove the mono Status enum and use the Windows constants instead in all Unix code. * Move all files into the regular directory structure. Suffix them with ".Unix" and ".Windows" when there are collisions. * Tiny bit of code cleanup * Add conditionals for recent versions of mono * Remove duplicate tests. * Fix multiplying TextureBrush with a disposed matrix on Unix (dotnet#24109) * Fix multiplying TextureBrush with a disposed matrix on Unix * Enable another passing test * Fix accidentally committed file * Add an error code fixup to Bitmap.Unix.cs to match Windows. * Bump System.Drawing.Common.TestData to 1.0.6 (dotnet#24149) * Bump system.drawing.common.testdata to 1.0.6 * Fix path to test data * Fix whitespace * Cleanup - Add/simplify using statements of disposable resources (Graphics, Bitmap) * Validate HatchStyle passed to HatchBrush ctor * Delete accidentally duplicated HatchBrush tests in LinearGradientBrushTests * Remove references to historical Mono bug IDs * Renable some already passing tests * Remove Thread.Sleep workaround * Update BuildTools to prerelease-02020-01 (dotnet#24172) * Add thread-local based switch to opt-in to ManagedHandler * Address PR feedback * PR feedback * Fix memory map imports (dotnet#24176) * Fix memory map imports Imports lost the last error attribute. Add it back and change the results to be the more correct "bool". Tweak the usage based on the new return type. #24159 * Move spin wait * Remove unused FEATURE_RANDOMIZED_STRING_HASHING (dotnet#24178) * Adding System.Data.Odbc package and including in metapackage * Fix MultiplyTransform with a disposed brush * Fix for 2nd Connection hangs in Mirroring Environment (dotnet#24181) * Switch tests to use thread-local switch for ManagedHandler And as a result re-enable parallelism of the test suite, which on my machine reduces the running time of the outerloop tests from 150s to 45s. * Update CoreClr, CoreSetup, ProjectNTfs, ProjectNTfsTestILC, Standard to preview1-25720-03, preview1-25719-04, beta-25721-01, beta-25721-01, preview1-25721-01, respectively * CoreFx #22406 Span based APIs - Text Reader Writer (dotnet#23786) * [Drawing] Move remaining "Unix" files into regular directory structure * As part of this, the "Windows" version of ImageFormat.cs is now being used everywhere. This file just contains a bunch of GUID's that are not platform-specific in any way. * Change AsSpan to property Span and rename AsMemory to Memory * Add Metafile and MetaHeader tests based on Mono's System.Drawing unit tests * Update project file * MetafileTests: Remove duplicates, and re-enable tests on Unix which are now working. Delete duplicate MetaHeadertests * Rationalize using statements * Update project file * Fix Metafile exception behavior * Simplify tests, remove duplicate test * Don't catch an ArgumentException just to get a NullReferenceException instead. * Assert.False(Object.ReferenceEquals => Assert.NotSame * PR feedback * Remove duplicate tests, fix typo * Remove X11 dependency for tests which are enabled on Unix. * Workaround libgdiplus glitches Don't test metafileHeader.Bounds when using libgdiplus. Don't test metafile.GetBounds on Unix, force MetafileHeader.Bounds to return an empty rectangle when MetafileSize == 0 Don't validate graphicsUnit on Unix. * Move Syndication to root/src * Disable Build of S.SM.Syndication. * Increase file descriptor limit in S.N.Http tests on macOS * Expose/test String.Create span-based method (dotnet#23872) * Expose/test String.Create span-based method * Address PR feedback * Update build to clang/llvm 3.9 (dotnet#24177) Update scripts, docs and build pipeline docker images to clang/llvm/lldb 3.9 * Update ProjectNTfs, ProjectNTfsTestILC, Standard to beta-25722-00, beta-25722-00, preview1-25722-01, respectively (dotnet#24207) * Remove the line that will copy the generated serializer to the pack. (dotnet#24199) * Revert "Update build to clang/llvm 3.9 (dotnet#24177)" This reverts commit 21e008a. * Remove stale SetStateMachine call in test * Ssl Stream Async Write (dotnet#23715) * Change from APM to Async/Await for write side * Added nameof * Reacting to review * Reacting to review * SSLStream : Fixed spelling mistake in file name (extra a) (dotnet#24221) * Fixed spelling mistake in file name * Fix csproj * Update ProjectNTfs, ProjectNTfsTestILC to beta-25723-00, beta-25723-00, respectively (dotnet#24222)
* Change from APM to Async/Await for write side * Added nameof * Reacting to review * Reacting to review Commit migrated from dotnet/corefx@6805793
/cc @stephentoub @geoffkizer @benaadams
This needs a bit of tidy still, but I want to check the idea, naming, if you are all cool with it I will start benchmarking and checking off any previous issues.
I used a single struct for the write side as that way I really only need a single pointer in the sync case and a pointer for the sslState and the cancellation token on the async side. It also means I don't have to drag that token around but get cancellation :)