Code files simplification suggestion

[eriawan]

hi guys,

After I cloned CoreWCF and opened the solution using Visual Studio, I see some linked files repeated everywhere:

Why are the projects organized like this? I know the magic is in the Directory.Build.targets file, but it is not quite easy to reason by directly looking at the csproj, because the linked files weren't there.

These "Common" files I think should be just included as part of CoreWCF.Primitives project, because it is used by both CoreWCF.Http and CoreWCF.NetTcp.

It is also easier for me to maintain. If my feedback is accepted, I'd like to submit PR to include those files in "Common" into CoreWCF.Primitives project.

cc @tibor19

[mconnew]

There are internal classes such as exception helpers which are needed by all of the projects which live there. Eventually things like ETW tracing will also live there. One possibility is to create a CoreWCF.Private package which the other packages depend on. It's possible to mark a nuget package that it's not meant to be directly added. I can't remember the consequences of what that does but I know it's really difficult to accidentally add the package on it's own. You can look at the System.Private.ServiceModel package to get an idea how that's done.

If you are interested in doing the work to make a CoreWCF.Private package, let me know.

[eriawan]

@mconnew

Thanks for the offer! I think I'm going to do this work start from tomorrow.

[mconnew]

@eriawan, are you still interested in working on this?

[eriawan]

@mconnew Yes, I'm still interested in working on this. Apologize, I was busy with something else and my work on this was buried.

Ok, I'll also try to look at the way System.Private.ServiceModel as well.

I'll try to submit an in progress PR next week,. Thanks for the reminder! 🙏

Update: PS: About the work that has been done in System.Private.ServiceModel, could I discuss with you if I have something to ask? Thanks in advance!

[mconnew]

Yes, ping me on gitter (same alias) and we can work out the best way to discuss. So that I don't forget, there's an interesting feature of the System.Private.ServiceModel package where it has a dummy file called . in the ref folder. This removes the ability for any of the types to be referenced in code by anyone adding the package directly so prevents problems with people referencing types which change. Basically allows you to create public types which aren't compilable against by end user packages, but can be referenced by other packages built at the same time.

[BradBarnich]

Why are three projects anyway? Since it is one in both the referencesource and the dotnet/wcf versions

[mconnew]

Three reasons. First, a large goal of .NET Core is to reduce the load time and size of applications. The minimum memory footprint for WCF in .NET Framework is System.ServiceModel.dll and System.ServiceModel.Internals.dll which combined are around 6.5MB. If you have a NetTcp service, you don't want to carry the overhead of MSMQ, NamedPipes, NetTcpPeer, Udp and all the various obscure bindings. I anticipate eventually CoreWCF will have a wider range of bindings than WCF. For example, I plan for the messaging transport to be generic and easily extended for other messaging systems beyond MSMQ such as AMQP, MQTT etc.

Second, there are going to be pieces which end up being Windows only. A simple example is MSMQ. A more nuanced and complicated example is providing the same kind of functionality as NamedPipes. There are 2 different ways to do it. There's using actual named pipes, and there's using Unix Domain Sockets (UDS). ASP.NET Core natively supports listening on UDS so it's only a small adaptation of the NetTcpBinding to do this. But there's no client side WCF UDS so that needs to be written too. To do actual NamedPipe's for Linux is going to require quite a bit of work to try and get it working cleanly, but that would also require writing the Linux version of the NamedPipe client binding. What's the point in doing that if there's UDS on Linux? But on Windows you want to do NamedPipes to interop with existing systems. So there's a reasonable chance that NamedPipeBinding (so CoreWCF.NamedPipe package) will be Windows only, and on Linux there will be a UDS implementation to provide the equivalent capability.

Third, it keeps things honest on the public api's. There are multiple things in WCF where components that are supposedly implementing public api's are using internal api's to do their work. A simple example is you can't completely reimplement SslStreamSecurityBindingElement yourself as there's an internal interface IStreamUpgradeChannelBindingProvider needed to be able to support extended protection (some component of the TLS session is inserted into the SOAP message so the receive can verify if there's a MITM attack happening) that's implemented by SslStreamSecurityUpgradeProvider. By having separate packages, it forces required api surface to be public which then makes it easier for someone else to provide implementations of components. It forces good abstractions. There's too much breaking of abstraction layers in WCF and it often makes it difficult to follow the code.

[BradBarnich]

Thank you for the detailed answer. The separation was frustrating when trying to bring over new features to CoreWCF. I don't have the examples handy, but there were files that were duplicated in both Http and NetTcp, and files that were in one of the child packages that was depended on by new code that should be in 'primitives'.

I have been working with the public API surface of WCF for a long time, but untangling the implementation code is on another level and at the time this artificial separation felt like unnecessary complexity. It was helpful for you to point out that future child packages might have external or platform specific dependencies.

Regarding internal, I like the approach that projects like efcore have taken, with very sparing usage of internal keyword and instead opting for an internal namespace. There is nothing more frustrating than running into something internal after you found the right extension point to implement what you need to (like IStreamUpgradeChannelBindingProvider 😄). Right now we are vendoring CoreWCF, so it's not a huge deal, but after things stabilize it would be nice to move to the shipped packages.

[mconnew]

The ASP.NET Core team tried to do the internal namespace thing too. The problem is having it public still results in people using it which then stops you from modifying internal implementations. My preference is to reduce the amount of internal stuff as much as possible and just make things public where applicable. A great example is the EnvelopeVersion and AddressingVersion classes. There's a bunch of useful internal properties which are used all through the code base to inform various components about strings needing to be used in the SOAP messages. For example there's AddressingVersion.Anonymous which is used to set the To SOAP header for when using anonymous addressing for the reply (such as when using HTTP reply where the response message is naturally tied to the request so no need to address the message).

Some of the stuff in the code might be a bit confusing for why I did it a particular way. There's a bit of history, this code base started off as a hacked up prototype. The intention of how the code would work with everything else changed multiple times. Eg should it be System.ServiceModel namespace, should we build on top of the client libraries which carry a lot of the primitives classes, should it consume ASP.NET Core or should ASP.NET Core consume this. This code morphed a few times with different ways it could be released. So there's still some left over patterns based on past assumptions which are now irrelevant. Don't be shy about suggesting improvements. For instance, making the AddressingVersion and EnvelopVersion api's public to remove some helpers I think I wrote to make them usable in other packages. My main heuristic is whether the api is the same shape as on .NET Framework and it's unlikely to change and neither is it's behavior.

The main candidates for putting into a Private package are things like metrics and ETW eventing as well as common helpers. Things which aren't related to actually sending messages back and forth, but are part of the infrastructure for how CoreWCF gets things done, but also aren't things which an external extensibility would use. For example, a 3rd party transport should have it's own ETW provider.