Skip to content

Commit

Permalink
Merge pull request dotnet#1571 from roncain/custom_channel_fix
Browse files Browse the repository at this point in the history 
Fix OnOpen/OnBeginOpen issues with custom channels
  • Loading branch information
@roncain
roncain authored Oct 10, 2016
2 parents 3cb666e + 96e0c4d commit d92f922
Show file tree
Hide file tree
Showing 17 changed files with 1,509 additions and 351 deletions.
14 changes: 14 additions & 0 deletions src/System.Private.ServiceModel/src/Internals/System/Runtime/TaskHelpers.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,20 @@ public static IDisposable RunTaskContinuationsOnOurThreads()
return new SyncContextScope();
}

// Calls the given Action asynchronously.
public static async Task CallActionAsync<TArg>(Action<TArg> action, TArg argument)
{
using (var scope = TaskHelpers.RunTaskContinuationsOnOurThreads())
{
if (scope != null) // No need to change threads if already off of thread pool
{
await Task.Yield(); // Move synchronous method off of thread pool
}

action(argument);
}
}

private class SyncContextScope : IDisposable
{
private readonly SynchronizationContext _prevContext;
Expand Down
4 changes: 4 additions & 0 deletions src/System.Private.ServiceModel/src/Resources/System.Private.ServiceModel.rd.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@

<Assembly Name="System.Private.ServiceModel">
<Namespace Name="System.ServiceModel">
<!-- All ICommunicationObject implementations require reflection -->
<Type Name="ICommunicationObject" Dynamic="Required All" >
<Subtypes Browse="All" />
</Type>
<Type Name="BasicHttpBinding" Dynamic="Required All" />
<!-- WCF attributes must be reflectable to instantiate Attribute from CustomAttributeData -->
<Type Name="FaultContractAttribute" Dynamic="Required All" >
Expand Down
20 changes: 2 additions & 18 deletions src/System.Private.ServiceModel/src/System/ServiceModel/ChannelFactory.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -319,15 +319,7 @@ internal protected override async Task OnCloseAsync(TimeSpan timeout)
{
if (_innerFactory != null)
{
IAsyncChannelFactory asyncFactory = _innerFactory as IAsyncChannelFactory;
if (asyncFactory != null)
{
await asyncFactory.CloseAsync(timeout);
}
else
{
_innerFactory.Close(timeout);
}
await CloseOtherAsync(_innerFactory, timeout);
}
}

Expand All @@ -345,15 +337,7 @@ protected internal override async Task OnOpenAsync(TimeSpan timeout)
{
if (_innerFactory != null)
{
IAsyncChannelFactory asyncFactory = _innerFactory as IAsyncChannelFactory;
if (asyncFactory != null)
{
await asyncFactory.OpenAsync(timeout);
}
else
{
_innerFactory.Open(timeout);
}
await OpenOtherAsync(_innerFactory, timeout);
}
}

Expand Down
21 changes: 11 additions & 10 deletions src/System.Private.ServiceModel/src/System/ServiceModel/Channels/ChannelFactoryBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,18 +198,19 @@ private async Task OnCloseAsyncInternal(TimeSpan timeout)
TimeoutHelper timeoutHelper = new TimeoutHelper(timeout);
foreach (IChannel channel in currentChannels)
{
IAsyncCommunicationObject iAsyncChannel = channel as IAsyncCommunicationObject;
if (iAsyncChannel != null)
{
await iAsyncChannel.CloseAsync(timeoutHelper.RemainingTime());
}
else
{
channel.Close(timeoutHelper.RemainingTime());
}
await CloseOtherAsync(channel, timeoutHelper.RemainingTime());
}

await Task.Factory.FromAsync(_channels.BeginClose, _channels.EndClose, timeout, TaskCreationOptions.None);
// CommunicationObjectManager (_channels) is not a CommunicationObject,
// so just choose existing synchronous or asynchronous close
if (_isSynchronousClose)
{
await TaskHelpers.CallActionAsync(_channels.Close, timeoutHelper.RemainingTime());
}
else
{
await Task.Factory.FromAsync(_channels.BeginClose, _channels.EndClose, timeoutHelper.RemainingTime(), TaskCreationOptions.None);
}
}
}
}
211 changes: 143 additions & 68 deletions src/System.Private.ServiceModel/src/System/ServiceModel/Channels/CommunicationObject.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,10 @@ public abstract class CommunicationObject : ICommunicationObject, IAsyncCommunic
private bool _traceOpenAndClose;
private object _eventSender;
private CommunicationState _state;
private bool _onOpenAsyncCalled;
private bool _onCloseAsyncCalled;
private bool _isSynchronousOpen;
private bool _isSynchronousClose;
internal bool _isSynchronousOpen;
internal bool _isSynchronousClose;
private bool _supportsAsyncOpenClose;
private bool _supportsAsyncOpenCloseSet;

protected CommunicationObject()
: this(new object())
Expand All @@ -44,6 +44,55 @@ protected CommunicationObject(object mutex)
_state = CommunicationState.Created;
}

// External CommunicationObjects cannot support IAsyncCommunicationObject,
// but can appear to because they may derive from WCF types that do.
// Attempting to call any IAsyncCommunicationObject method on those types
// will go directly to the WCF base type and bypass the external type's
// synchronous or asynchronous code paths. We cannot distinguish between
// this and a product CommunicationObject handling it and calling base.
// This property detects if the current type is safe to use for
// IAsyncCommunicationObject method calls.
internal bool SupportsAsyncOpenClose
{
get
{
if (!_supportsAsyncOpenCloseSet)
{
// We'll use the simple heuristic that namespace System.ServiceModel
// indicates a product type that is required to support async open/close.
// We don't expect exceptions here in NET Native because the product rd.xml
// grants the Reflection degree to all subtypes of ICommunicationObject.
// However, in the interests of being safe, catch that exception if it happens.
try
{
string ns = this.GetType().Namespace;
_supportsAsyncOpenClose = ns != null && ns.StartsWith("System.ServiceModel");
}
catch
{
// The most likely situation for this exception is the NET Native
// toolchain not recognizing this type is ICommunicationObject.
// But in that case, the best assumption is that this is a WCF
// product type, and therefore it supports async open/close.
_supportsAsyncOpenClose = true;
}

_supportsAsyncOpenCloseSet = true;
}

return _supportsAsyncOpenClose;
}
set
{
// It is permissible for types to set this value if they know
// they can or cannot support async open/close. Unsealed public
// types must *not* set this to true because they cannot know if
// an external derived type does support it.
_supportsAsyncOpenClose = value;
_supportsAsyncOpenCloseSet = true;
}
}

internal bool Aborted
{
get { return _aborted; }
Expand Down Expand Up @@ -196,7 +245,7 @@ async Task IAsyncCommunicationObject.CloseAsync(TimeSpan timeout)
if (!_onClosingCalled)
throw TraceUtility.ThrowHelperError(this.CreateBaseClassMethodNotCalledException("OnClosing"), Guid.Empty, this);

await OnCloseAsync(actualTimeout.RemainingTime());
await OnCloseAsyncInternal(actualTimeout.RemainingTime());

OnClosed();
if (!_onClosedCalled)
Expand All @@ -223,6 +272,35 @@ async Task IAsyncCommunicationObject.CloseAsync(TimeSpan timeout)
}
}

// Internal helper to call the right form of the OnClose() method
// asynchronously. It depends on whether the type can support the
// async API's and how the current Communication object is being closed.
private async Task OnCloseAsyncInternal(TimeSpan timeout)
{
// If this type is capable of overriding OnCloseAsync,
// then use it for both async and sync closes
if (SupportsAsyncOpenClose)
{
// The class supports OnCloseAsync(), so use it
await OnCloseAsync(timeout);
}
else
{
// This type is an external type that cannot override OnCloseAsync.
// If this is a synchronous close, invoke the synchronous OnClose)
if (_isSynchronousClose)
{
await TaskHelpers.CallActionAsync<TimeSpan>(OnClose, timeout);
}
else
{
// The class does not support OnCloseAsync, and this is an asynchronous
// close, so use the Begin/End pattern
await Task.Factory.FromAsync(OnBeginClose, OnEndClose, timeout, TaskCreationOptions.RunContinuationsAsynchronously);
}
}
}

private Exception CreateNotOpenException()
{
return new InvalidOperationException(SR.Format(SR.CommunicationObjectCannotBeUsed, this.GetCommunicationObjectType().ToString(), _state.ToString()));
Expand Down Expand Up @@ -353,8 +431,7 @@ async Task IAsyncCommunicationObject.OpenAsync(TimeSpan timeout)
if (!_onOpeningCalled)
throw TraceUtility.ThrowHelperError(this.CreateBaseClassMethodNotCalledException("OnOpening"), Guid.Empty, this);

TimeSpan remainingTime = actualTimeout.RemainingTime();
await OnOpenAsync(remainingTime);
await OnOpenAsyncInternal(actualTimeout.RemainingTime());

OnOpened();
if (!_onOpenedCalled)
Expand All @@ -371,6 +448,34 @@ async Task IAsyncCommunicationObject.OpenAsync(TimeSpan timeout)
}
}

// Internal helper to call the right form of the OnOpen() method
// asynchronously. It depends on whether the type can support the
// async API's and how the current Communication object is being opened.
private async Task OnOpenAsyncInternal(TimeSpan timeout)
{
// If this type is capable of overriding OnOpenAsync,
// then use it for both async and sync opens
if (SupportsAsyncOpenClose)
{
// The class supports OnOpenAsync(), so use it
await OnOpenAsync(timeout);
}
else
{
// This type is an external type that cannot override OnOpenAsync.
// If this is a synchronous open, invoke the synchronous OnOpen)
if (_isSynchronousOpen)
{
await TaskHelpers.CallActionAsync<TimeSpan>(OnOpen, timeout);
}
else
{
// The class does not support OnOpenAsync, so use the Begin/End pattern
await Task.Factory.FromAsync(OnBeginOpen, OnEndOpen, timeout, TaskCreationOptions.RunContinuationsAsynchronously);
}
}
}

protected virtual void OnClosed()
{
_onClosedCalled = true;
Expand Down Expand Up @@ -765,81 +870,51 @@ internal void ThrowPending()

internal protected virtual Task OnCloseAsync(TimeSpan timeout)
{
#if !FEATURE_NETNATIVE // Avoid Reflection in NET Native and rely on CoreCLR to catch this problem.
// All product types are required to override this method
Contract.Assert(String.IsNullOrEmpty(GetType().Namespace) ||
!GetType().Namespace.StartsWith("System.ServiceModel"),
String.Format("Type '{0}' is required to override OnCloseAsync", GetType()));
#endif
// The code below executes only for custom CommunicationObjects because
// all WCF product code is required to override OnCloseAsync() and not to call
// this base implementation.

// If we have already executed this code path, just return a completed Task.
// This is a safeguard against product code that failed to override OnCloseAsync().
if (_onCloseAsyncCalled)
{
return TaskHelpers.CompletedTask();
}

_onCloseAsyncCalled = true;

// External types cannot override this method because it is not public.
// If the caller started this open synchronously, we must use the synchronous
// methods they may have overridden, but we call then asynchronously.
if (_isSynchronousClose)
{
return CallActionAsync<TimeSpan>(OnClose, timeout);
}
// All WCF product types are required to override this method and not call base.
// No external types will be able to reach here because it is not public.
Contract.Assert(false, String.Format("Type '{0}' is required to override OnCloseAsync", GetType()));

// This began as an asynchronous close, so redirect this call to the APM path.
return Task.Factory.FromAsync(OnBeginClose, OnEndClose, timeout, TaskCreationOptions.RunContinuationsAsynchronously);
return TaskHelpers.CompletedTask();
}

internal protected virtual Task OnOpenAsync(TimeSpan timeout)
{
#if !FEATURE_NETNATIVE // Avoid Reflection in NET Native and rely on CoreCLR to catch this problem.
// All product types are required to override this method
Contract.Assert(String.IsNullOrEmpty(GetType().Namespace) ||
!GetType().Namespace.StartsWith("System.ServiceModel"),
String.Format("Type '{0}' is required to override OnOpenAsync", GetType()));
#endif
// The code below executes only for custom CommunicationObjects because
// all WCF product code is required to override OnOpenAsync() and not to call
// this base implementation.
// All WCF product types are required to override this method and not call base.
// No external types will be able to reach here because it is not public.
Contract.Assert(false, String.Format("Type '{0}' is required to override OnOpenAsync", GetType()));

// If we have already executed this code path, just return a completed Task.
// This is a safeguard against product code that failed to override OnOpenAsync().
if (_onOpenAsyncCalled)
{
return TaskHelpers.CompletedTask();
}

_onOpenAsyncCalled = true;
return TaskHelpers.CompletedTask();
}

// External types cannot override this method because it is not public.
// If the caller started this open synchronously, we must use the synchronous
// methods they may have overridden, but we call then asynchronously.
// Helper used to open another CommunicationObject "owned" by the current one.
// It is used to propagate the use of either the synchronous or asynchronous methods
internal async Task OpenOtherAsync(ICommunicationObject other, TimeSpan timeout)
{
// If the current object is being opened synchronously, use the synchronous
// open path for the other object.
if (_isSynchronousOpen)
{
return CallActionAsync<TimeSpan>(OnOpen, timeout);
await TaskHelpers.CallActionAsync<TimeSpan>(other.Open, timeout);
}
else
{
await Task.Factory.FromAsync(other.BeginOpen(timeout, callback: null, state: null), other.EndOpen);
}

// This began as an asynchronous open, so just redirect this call to the APM path.
return Task.Factory.FromAsync(OnBeginOpen, OnEndOpen, timeout, TaskCreationOptions.RunContinuationsAsynchronously);
}

// Calls the given Action asynchronously.
private async Task CallActionAsync<TArg>(Action<TArg> action, TArg argument)
// Helper used to close another CommunicationObject "owned" by the current one.
// It is used to propagate the use of either the synchronous or asynchronous methods
internal async Task CloseOtherAsync(ICommunicationObject other, TimeSpan timeout)
{
using (var scope = TaskHelpers.RunTaskContinuationsOnOurThreads())
// If the current object is being closed synchronously, use the synchronous
// close path for the other object.
if (_isSynchronousClose)
{
if (scope != null) // No need to change threads if already off of thread pool
{
await Task.Yield(); // Move synchronous method off of thread pool
}

action(argument);
await TaskHelpers.CallActionAsync<TimeSpan>(other.Close, timeout);
}
else
{
await Task.Factory.FromAsync(other.BeginClose(timeout, callback: null, state: null), other.EndClose);
}
}
}
Expand Down
7 changes: 7 additions & 0 deletions ...erviceModel/src/System/ServiceModel/Channels/ConnectionOrientedTransportChannelFactory.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,13 @@ internal ConnectionOrientedTransportChannelFactory(
// there is the binding is configured with security
_flowIdentity = supportsImpersonationDuringAsyncOpen;
}

// We explicitly declare this type and all derived types support
// async open/close. We currently must do this because the NET Native
// toolchain does not recognize this type was granted Reflection degree.
// Is it safe to do this only because this is an internal type and no
// derived type is public or exposed in contract.
SupportsAsyncOpenClose = true;
}

public int ConnectionBufferSize
Expand Down
Loading

0 comments on commit d92f922

Please sign in to comment.