Initial NetNamedPipe implementation #4955
Conversation
|
@ericstj can you help with getting |
|
Have a look at this wcf/eng/FacadeAssemblies.targets Lines 45 to 63 in a285684 It's only included for facade assemblies Line 4 in a285684 So it's not set for your net6.0 or net6.0-windows TargetFrameworks. This part of your FacadeAssemblies.targets is both finding the reference assembly to include in the package and setting it for You have reference assemblies in other WCF packages because you have different reference identity than implementation due to System.Private.ServiceModel. If you don't need that here you could delete the reference assemblies. That would simplify this project quite a bit. I think this was what @ViktorHofer's was reccomending. Rather than using a platform not-supported assembly for net6.0 why not just build a normal assembly for net6.0 and then throw in APIs you can't support on non-windows and annotate them with SupportedOSPlatformAttribute. |
| <IsPackable>true</IsPackable> | ||
| <IsShipping>$(Ship_WcfPackages)</IsShipping> | ||
| <!-- TODO: Make this net462 and netstandard2.0 after it's multi-targetting .NET Framework --> | ||
| <TargetFrameworks>net6.0;net6.0-windows;net461</TargetFrameworks> |
There was a problem hiding this comment.
It looks to me like your net461 implementation is a full facade. Since you aren't exposing any types on that TargetFramework and you don't have a netstandard2.0 assembly with types that you need to replace with forwards on .NETFramework , could you just delete your net461 build?
There was a problem hiding this comment.
FYI, this PR removes any implementation living in System.Private.ServiceModel. We're getting rid of it.
If I remove the Ref project and build this project for net6.0 and net6.0-windows only, won't that make this package unusable on .NET Framework? I know there are customers who multi target between NetFx and .Net.
Previously our packages had reference assemblies and implementations for net461, netcoreapp3.1, net60 (needed for MAUI due to Xamarin suppression in our package having higher precedence that netcoreapp3.1), and netstandard2.0.
With .NET Core 3.1 about to go out of support, there's no need for us to have netstandard2.0. net461 and net6.0 should cover us.
There was a problem hiding this comment.
@ericstj, are you suggesting the assembly shouldn't be supported on net461? The intention was that the public api's which exist on .NET Framework will type forward to System.ServiceModel on .NET Framework. The idea is that a consumer can multi target to .NET Framework and .NET 6 and use the same package.
Is that a waste of time and it would be better to have the consumer do the conditional stuff in their project to reference System.ServiceModel when compiling for .NET Framework and only reference the nuget package on .NET 6?
The reason for not targeting netstandard2.0 is that the implementation would only run on .NET 6+ (as 3.1 is about to go out of support and .NET Framework carries its own) and it frees me up to use newer apis. I'd be fine with a reference assembly targeting netstandard2.0 as I won't be exposing any newer types on the public api surface, just want to consume them in the implementation.
There was a problem hiding this comment.
The reason for not targeting netstandard2.0 is that the implementation would only run on .NET 6+ (as 3.1 is about to go out of support and .NET Framework carries its own) and it frees me up to use newer apis. I'd be fine with a reference assembly targeting netstandard2.0 as I won't be exposing any newer types on the public api surface, just want to consume them in the implementation.
I know you were asking Eric for an opinion but let me quickly share mine. These days, with multi-targeting being a first-class concept I wouldn't include a .NETFramework facade assembly just for the sake of a customer not needing to apply a condition when targeting both modern .NET and .NET Framework and referencing this package. We have been doing this in corefx/runtime for a while, but only because we also offer .NETStandard assets. In this case, the package targets modern .NET only, and that's OK and future proof.
There was a problem hiding this comment.
They can already multi-target without a facade. The facade is only needed if you defined a different assembly location in an assembly that is compatible with NETframework (EG: A netstandard2.0 assembly, or a portable-class-library assembly). Since you aren't doing that you don't need a facade on .NETFramework -- just let folks reference System.ServiceModel to get the types.
There was a problem hiding this comment.
@ericstj, @ViktorHofer, what are your thoughts on the ServiceModel shim? Its purpose was to allow you to use an assembly compiled for .NET Framework on .NET Core. I'm having problems getting the shim to be generated with the removal of netstandard. Should I put in the work to get that working or drop it? Is that still an important scenario now?
There was a problem hiding this comment.
IMHO I would drop it as running .NET Framework compiled assemblies on modern .NET was never a "first class" supported scenario. When you do a "dotnet add reference" pointing to a .NET Framework project you will even get an error today:
C:\temp\modernent>dotnet add reference ..\netfx\
Project `C:\temp\netfx\netfx.csproj` cannot be added due to incompatible targeted frameworks between the two projects. Review the project you are trying to add and verify that is compatible with the following targets:
- net7.0
| SR.Format(SR.PipeConnectFailed, remoteUri.AbsoluteUri), innerException); | ||
| } | ||
|
|
||
| public async ValueTask<IConnection> ConnectAsync(Uri remoteUri, TimeSpan timeout) |
There was a problem hiding this comment.
There are a lot of methods that pass around a TimeSpan timeout. Have you considered using a CancellationToken instead? That's the idiomatic way to do cancellation with Tasks.
There was a problem hiding this comment.
Passing a CancellationToken hides the original timeout. It means the backoff timer helper can't do its job. The backoff timer is given the timeout value which it sets as the deafline. It starts with an initial timeout of 1 ms. If connection fails, it waits this amount of time and tries again. On the next wait, it doubles the amount of time it waits. Ie 1ms, 2ms, 4ms and so on (some randomness is thrown in to prevent multiple clients synchronizing). If the next timeout would take you past the deadline, it truncates the timeout to occur at the deadline. Switching to a CancellationToken would mean we couldn't do that last retry as we would only know we have run out of time once the token has been cancelled. You can't register a cancellation callback and do one final retry as that would take you past the deadline. The code actually subtracts 150ms from the initial timeout (see PrepareConnect) so that the final backoff delay won't actually go over the original timeout.
We do only lose the final retry, so this might still be worth considering, but it is a change in behavior.
|
|
||
| private async ValueTask<IConnection> TryConnectAsync(Uri remoteUri, string resolvedAddress, BackoffTimeoutHelper backoffHelper) | ||
| { | ||
| bool lastAttempt = backoffHelper.IsExpired(); |
There was a problem hiding this comment.
It seems odd that this method still attempts to connect after timeout
There was a problem hiding this comment.
See my previous comment, the final delay time is truncated to hit the originally specified timeout exactly. IsExpired means we just hit the deadline provided to the backoff helper. This had 150ms subtracted from it in PrepareConnect so we're actually up to 150ms before the original timeout (there's some wiggle room because of system timer resolution, so likely between 135ms and 150ms).
| try | ||
| { | ||
| namedPipeClient = new NamedPipeClientStream(".", resolvedAddress, PipeDirection.InOut, PipeOptions.Asynchronous, TokenImpersonationLevel.Anonymous, HandleInheritability.None); | ||
| await namedPipeClient.ConnectAsync((int)backoffHelper.OriginalTimeout.TotalMilliseconds); |
There was a problem hiding this comment.
Shouldn't this be the remaining time? Maybe I don't understand how timeouts work here. I would expect that the method immediately throws as soon as a timeout is exceeded.
There was a problem hiding this comment.
You're right that this is wrong. I looked at the implementation and it tries to connect to the named pipe, but that requires the service side to have called ConnectNamedPipe to have a pending connection. If that's not the case the connect fails and it calls WaitNamedPipe with the timeout. This synchronously blocks until ConnectNamedPipe is called by the service. If I pass zero, it uses the what was specified when the service created the pipe, which defaults to 50ms (and WCF doesn't set it to anything explicit). I'm going to pass 1 so that it doesn't hold a thread blocking waiting for the service to begin listening.
|
|
||
| internal unsafe class PipeSharedMemory : IDisposable | ||
| { | ||
| internal const string PipeLocalPrefix = @"Local\"; |
There was a problem hiding this comment.
I noticed that named pipes on my computer tend to capitalize LOCAL\. I'm guessing it doesn't matter.
There was a problem hiding this comment.
This was copied from WCF in NetFx so I think you're right. I suspect it's the whole case preserving case insensitive thing that Windows does for its file system by default.
| } | ||
| } | ||
|
|
||
| public async ValueTask<int> ReadAsync(Memory<byte> buffer, TimeSpan timeout) |
There was a problem hiding this comment.
+1 to CancellationToken instead of TimeSpan timeout.
There was a problem hiding this comment.
We use IOThreads for running our own timer queue baseed on a Fibonacci priority queue as we had massive performance problems with the runtime's TimerManager (global lock around cancelling timers which CancellationToken does). Some of our perf tests were spending 30%+ CPU time registering and cancelling tiemrs. This was code copied from NetFx. I could rework that to create our own equivalent of a CancellationTokenSource which uses our own timer mechanism under the hood. We do have a problem of when a CancellationToken gets cancelled that we need to throw an exception with the original Timeout and that's not easily retrieved from a CancellationToken. I'll give this some thought and try to come up with a solution.
| { | ||
| ValidateBufferBounds(buffer); | ||
| TimeoutHelper timeoutHelper = new TimeoutHelper(timeout); | ||
| var cancellationToken = await timeoutHelper.GetCancellationTokenAsync(); |
There was a problem hiding this comment.
Why is GetCancellationTokenAsync async? Seems like an odd place to do IO
There was a problem hiding this comment.
It's not doing IO, it's coalescing CancellationToken's based on deadline. This was a mitigation to reduce load on the TimerManager. As two threads could be requesting a CancellationToken for the same deadline (within a window), the second thread that tries to get an instance will have to wait for the first one to finish initializing it. Think of it like an asynchronous version of Lazy<T> where the loser of the race condition doesn't block waiting for the winner to finish.
| } | ||
| else if (existingReadIsPending) | ||
| { | ||
| if (!TimeoutHelper.Wait(_atEOFEvent, timeoutHelper.RemainingTime())) |
There was a problem hiding this comment.
Should EOFEvent ManualResetEventSlim be a TaskCompletionSource? It looks like this will block. A TCS would allow an await here.
| await WaitForWriteZero(writeValueTask, timeout, false); | ||
|
|
||
| // wait for read to complete/fail | ||
| await WaitForReadZero(readValueTask, timeout, false); |
There was a problem hiding this comment.
timeout doesn't look like it is used by these methods
There was a problem hiding this comment.
NVM, it is used in message.
If you go down a path of changing timeouts to CancellationTokens, I would expect at a higher-level method that does take a timeout, you would catch OperationCanceledException and then check if the passed in timeout has been exceeded. If has, then you could wrap the OperationCanceledException in a TimeoutException and add a message with the timeout in it.
There was a problem hiding this comment.
The caller won't know what's the appropriate exception message should be as we have different exception messages for socket etc. I could catch the OperationCanceledException, catch it and throw TimeoutException with the literal {0} where the timeout should be, have the caller catch TimeoutException, reformat the string with the timeout and then rethrow again. It seems wrong to throw 3 times to achieve this though. Do you have any thoughts about a more elegant way to solve this?
| if (shouldCloseHandle) | ||
| { | ||
| CloseHandle(false, null, TransferOperation.Undefined); | ||
| } |
There was a problem hiding this comment.
nit: Does handle come from old code where native pipe types were used directly? I'd change handle to pipe in names.
|
|
||
| private void EnterReadingState() | ||
| { | ||
| _inReadingState = true; |
There was a problem hiding this comment.
nit: Debug.Assert(Monitor.IsEntered(_readLock)); is a simple way to validate methods like these are always called in the correct lock
mconnew
force-pushed
the
NetNamedPipe
branch
2 times, most recently
from
f1b9f1b to
a2882d4
Compare
| _closeState = CloseState.Open; | ||
| _exceptionEventType = TraceEventType.Error; | ||
| _connectionBufferSize = connectionBufferSize; | ||
| _atEOFTask = new TaskCompletionSource(); |
There was a problem hiding this comment.
You probably want this:
| _atEOFTask = new TaskCompletionSource(); | |
| _atEOFTask = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); |
Kind of related, does WCF care about the synchronization context? Should ConfigureAwait(false) be added to all awaits?
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Things other than NetNamedPipe that are included in this PR:
Still a little bit of work needed