SSLStream : Removed sync lock method#24352
Conversation
|
@dotnet-bot Test Outerloop Windows x64 Debug Build |
|
All failures in the OuterLoop Linux are again with System.Drawing.
Tests that fail are in MonoTests.System.Drawing.Imaging.PngCodecTest, all failing so tests are
And that repeated up to 4bit. I would hazard a guess and say this is unrelated. /cc @mellinoe @safern any ideas why System.Drawing seems to be failing in outerloop a lot? |
|
As a double check there is no differences in benchmarks that I can see. (Other than Azure "wobble") |
The benchmark uses the synchronous methods? |
|
No actually will make one that does. Getresult on completed task should pretty much be a no-op right? And as there isn't even a way to force renegotiation in .net .... |
Yup (other than if it failed, in which case it'll throw). |
|
Right finally forced myself to write some Sync networking code, the results I see are 44906.5 for the "new" version vs "44705.7" for the "old" version, basically I can't find much in it, maybe slightly towards the new version but I wouldn't put money on either on any day |
| public Task LockAsync() | ||
| { | ||
| _sslState.CheckEnqueueWrite(); | ||
| _sslState.CheckEnqueueWriteAsync().GetAwaiter().GetResult(); |
There was a problem hiding this comment.
If this is just going to use GetAwaiter().GetResult() and then return CompletedTask, why not remove LockAsync from ISslWriteAdapter entirely and just have the current call sites to LockAsync use CheckEnqueueWriteAsync? Seems like the implementation here should really just be:
public Task LockAsync() => _sslState.CheckEnqueueWriteAsync();which is the same as the SslWriteAsync implementation, in which case it shouldn't be necessary in the abstraction.
There was a problem hiding this comment.
Because the call site is the same path for sync and async?
I see a single call to LockAsync:
https://github.com/Drawaes/corefx/blob/e415d485d542fe952d7a6bdd9257d5a8837448cc/src/System.Net.Security/src/System/Net/Security/SslStreamInternal.cs#L294
If CheckEnqueueWriteAsync actually completes synchronously, then it doesn't matter whether you call GetAwaiter().GetResult() or not, as the call site is going to get back an already completed task. If CheckEnqueueWriteAsync completes asynchronously, the call site is still capable of working with async operations, and we shouldn't be forcing it to block earlier than it otherwise needs to... the call site will treat it like any other incomplete async operation, and then we'll end up blocking at the top-most level if necessary: https://github.com/Drawaes/corefx/blob/e415d485d542fe952d7a6bdd9257d5a8837448cc/src/System.Net.Security/src/System/Net/Security/SslStreamInternal.cs#L114
That said, I'm still not entirely clear why it's ok to remove the sync path. Can you explain that in more detail? Are you saying the sync path today never actually blocks on anything?
There was a problem hiding this comment.
Nope but I am saying that in the Sync and Async case they both block waiting effectively for a different thread to complete the handshake. So the flow should be the same, its not like calling an async underlying method normally that will cause another task to be generated.
Now originally I made the sync and async versions because of the Async path that I made in the SslStream. But I have now taken a second look and because of the combined path I thought this was a good way to kill off some more code with no effect.
I have been known to be wrong though, so I am interested if you think there is a subtle difference I am missing here, and if I am then I am happy to drop this PR. Its really about killing off as much unneeded complexity, especially around this locking stuff in here.
There was a problem hiding this comment.
I am saying that in the Sync and Async case they both block waiting effectively for a different thread to complete the handshake. So the flow should be the same, its not like calling an async underlying method normally that will cause another task to be generated.
I'm looking at this code:
https://github.com/Drawaes/corefx/blob/e415d485d542fe952d7a6bdd9257d5a8837448cc/src/System.Net.Security/src/System/Net/Security/SslState.cs#L1324-L1333
This would seem to suggest that, today, a synchronous caller will be woken up directly by this code, whereas an asynchronous caller will be woken up asynchronously via a work item being queued. Doesn't that mean that with this PR, the synchronous caller will be blocked and the asynchronous work will need to queue another work item that'll need to run to unblock it?
There was a problem hiding this comment.
You might be right here, I won't close the PR for now so that the testing can be discussed below. But this might just be better refactored completely to remove the current stuff. I suspect a SemaphoreSlim with count = 1 would be ideal here.
|
Because the call site is the same path for sync and async? |
| { | ||
| // Proceed with write. | ||
| return; | ||
| } |
There was a problem hiding this comment.
It looks like we don't have any tests that make it beyond this return, in either the sync or async versions. Is it possible to add some?
There was a problem hiding this comment.
In a word no, @bartonjs should be able to confirm. But from .net I am sure there is no way to trigger a renegotiation. The testing I have done has been pretty manual using OpenSSL, or my own code to trigger the situations. So it might be possible to write an outerloop style scripted test but that would be beyond my knowledge of how your environment works for outerloop stuff...
There was a problem hiding this comment.
Ok, thanks. It's good to hear you've tested it manually, thanks, but it's scary to me that we have all this code to support renegotiation and nothing actually testing it automatically. @bartonjs, @Priya91, @geoffkizer, @wfurt, anything we can do about that?
There was a problem hiding this comment.
Yes I agree, if someone is happy to help me through how a scripted outloop test is created, I am more than happy to put something together that should test it, if help is wanted. (I also agree that its a bit worrying that there is zero test coverage for what is a complex bit of code).
|
Closing this for now, as there is no test coverage in this area and the benefits don't justify the risks of changing untested code. Have opened #24407 to ask for tests for this area |
8 participants
Looking at the code, because this method is basically never actually blocked on (only if a renegotiation takes place during a write) there is no need for a second "sync" method.
Instead we can block on the Async method as it is controlled via a TCS anyway and doesn't go down to a "real blocking method" at the OS level so no extra threads should be generated from the original. This again simplifies the code maintenance going forward and refactoring SslState