Make federation implementation async and fix memory leak #4481
Conversation
|
@brentschmaltz, can you take a look at this. I've tidied some things up in the process of fixing a couple of issues. |
...viceModel.Federation/src/System/IdentityModel/Security/WrapperSecurityCommunicationObject.cs
Outdated
Show resolved
Hide resolved
mconnew
force-pushed
the
AsyncFederation
branch
from
a38ddfd to
0fc7f7e
Compare
|
|
||
| namespace System.ServiceModel.Security | ||
| { | ||
| interface ISecurityCommunicationObject |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
mconnew
force-pushed
the
AsyncFederation
branch
from
0fc7f7e to
25854a4
Compare
| @@ -487,7 +487,7 @@ protected internal override async Task OnOpenAsync(TimeSpan timeout) | |||
| { | |||
| TimeoutHelper timeoutHelper = new TimeoutHelper(timeout); | |||
| SetupSessionTokenProvider(); | |||
| SecurityUtils.OpenTokenProviderIfRequired(_sessionTokenProvider, timeoutHelper.RemainingTime()); | |||
| await SecurityUtils.OpenTokenProviderIfRequiredAsync(_sessionTokenProvider, timeoutHelper.RemainingTime()); | |||
There was a problem hiding this comment.
should you have .ConfigureAwait(false) on these async methods?
There was a problem hiding this comment.
It's not needed. I hated the idea of littering my code everywhere with ConfigureAwait because it's messy, and it only takes missing one to create a difficult to find bug. So instead I have some code which is called when calling WCF using an async contract which checks if you are using a non-default sync context or a non-default task scheduler, and if you are, forces an artificial continuation into the worker thread pool. So this code is guaranteed to not need ConfigureAwait. ConfigureAwait is needed if you might go async. If you are guaranteed to always go async (and WCF always does as it always does network IO which requires a response), then you can just forcibly go async from the start.
There was a problem hiding this comment.
OK, cool just checking.
If you have a controlled call graph, you are probably OK,
We created a Roslyn Code Analyzer to check for our other libraries we have many entry points.
@mconnew What was the memory leak?
There was a problem hiding this comment.
The memory leak was not closing the channel. The channel factory will hold on to the channel in an internal list. Each call would create a new instance which would accumulate instances. The fix is to close the channel after usage. Also we now close the channel factory when the parent channel factory is closed as that closes the token provider.
* Fix memory leak and add async token getting impl * Make WSFed implementation async and fix memory leak Co-authored-by: Matt Connew <matt.connew@microsoft.com> Co-authored-by: Matt Connew <mconnew@microsoft.com> Co-authored-by: Matt Connew <matt.connew@microsoft.com>
Implemented BeginGetTokenCore and EndGetTokenCore to provide async implementation and fixed memory leak around ChannelFactory and channel lifetime.
Fixes #4462
Fixes #4461