Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

SSLStream : Removed sync lock method #24352

Closed
Drawaes wants to merge 1 commit into from
Closed

SSLStream : Removed sync lock method #24352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
33 changes: 0 additions & 33 deletions src/System.Net.Security/src/System/Net/Security/SslState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1303,39 +1303,6 @@ internal Task CheckEnqueueWriteAsync()
}
}

internal void CheckEnqueueWrite()
{
// Clear previous request.
_queuedWriteStateRequest = null;
int lockState = Interlocked.CompareExchange(ref _lockWriteState, LockWrite, LockNone);
if (lockState != LockHandshake)
{
// Proceed with write.
return;
}
Copy link
Copy Markdown
Member

@stephentoub stephentoub Oct 1, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Author

@Drawaes Drawaes Oct 1, 2017

Choose a reason for hiding this comment

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

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...

Copy link
Copy Markdown
Member

@stephentoub stephentoub Oct 1, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Author

@Drawaes Drawaes Oct 1, 2017

Choose a reason for hiding this comment

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

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).

#22998 (comment)


LazyAsyncResult lazyResult = null;
lock (this)
{
if (_lockWriteState != LockHandshake)
{
// Handshake has completed before we grabbed the lock.
CheckThrow(authSuccessCheck: true);
return;
}

_lockWriteState = LockPendingWrite;

lazyResult = new LazyAsyncResult(null, null, /*must be */null);
_queuedWriteStateRequest = lazyResult;
}

// Need to exit from lock before waiting.
lazyResult.InternalWaitForCompletion();
CheckThrow(authSuccessCheck: true);
return;
}

internal void FinishWrite()
{
int lockState = Interlocked.CompareExchange(ref _lockWriteState, LockNone, LockWrite);
Expand Down
2 changes: 1 addition & 1 deletion src/System.Net.Security/src/System/Net/Security/SslStreamInternal.Adapters.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ private struct SslWriteSync : ISslWriteAdapter

public Task LockAsync()
{
_sslState.CheckEnqueueWrite();
_sslState.CheckEnqueueWriteAsync().GetAwaiter().GetResult();
Copy link
Copy Markdown
Member

@stephentoub stephentoub Oct 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@stephentoub stephentoub Oct 1, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Author

@Drawaes Drawaes Oct 1, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@stephentoub stephentoub Oct 1, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Author

@Drawaes Drawaes Oct 1, 2017

Choose a reason for hiding this comment

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

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.

return Task.CompletedTask;
}

Expand Down