Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove System.ServiceModel.Primitives dependency. #1017

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

shanecelis
Copy link

In an effort to use NetMQ with Unity3D, one problem was trying to
satisfy the dependencies for System.ServiceModel.Primitives. It
required also providing System.ServiceModel.{Http,Duplex,NetTcp} and
perhaps more before I gave up. These dependencies were only noticed
while Unity3D built the project. When using NetMQ in a regular dotnet
project, I've had no problems with the System.ServiceModel.Primitives
dependencies.

I added an alternative implementation of IBufferPool using the
ArrayPool from System.Buffers but it is unused. I instrumented the
build system to have a "Flavor" that by default is set to "Standard",
which changes nothing from before.

For my usecase, however, I set the flavor to "Minimal".

$ dotnet build -p:Flavor=Minimal

This excludes "System.ServiceModel.Primitives" and defines a
preprocessor flag of FLAVOR_MINIMAL that excludes the
BufferManagerBufferPool class and uses the ArrayBufferPool instead.

Add an ArrayPool<byte> from the System.Buffers namespace. With it, one
can exclude the System.ServiceModel.Primitives package.
In an effort to use NetMQ with Unity3D, one problem was trying to
satisfy the dependencies for `System.ServiceModel.Primitives`. It
required also providing `System.ServiceModel.{Http,Duplex,NetTcp}` and
perhaps more before I gave up. These dependencies were only noticed
while Unity3D built the project. When using NetMQ in a regular dotnet
project, I've had no problems with the `System.ServiceModel.Primitives`
dependencies.

I added an alternative implementation of `IBufferPool` using the
`ArrayPool` from `System.Buffers` but it is unused. I instrumented the
build system to have a "Flavor" that by default is set to "Standard",
which changes nothing from before.

For my usecase, however, I set the flavor to "Minimal".

    $ dotnet build -p:Flavor=Minimal

This excludes "System.ServiceModel.Primitives" and defines a
preprocessor flag of `FLAVOR_MINIMAL` that excludes the
BufferManagerBufferPool class and uses the ArrayBufferPool instead.
@drewnoakes
Copy link
Member

Is there a reason not to outright replace the prior implementation? Losing the dependency on a WCF library seems sensible.

@shanecelis
Copy link
Author

shanecelis commented May 9, 2022

There's no reason that I know. I did it this way merely to make the least invasive change.

Technically using ArrayPool<T> introduces a dependency on System.Buffers but that's already a dependency that's required by System.Memory which NaCl.Net requires. Let me know if you want to drop the System.ServiceModel.Primitives dependency all together and get rid of the build flavor stuff.

@drewnoakes
Copy link
Member

I'd vote for just replacing the WCF implementation outright, and removing the dependency on that package.

The implementation is here for reference: https://github.com/CoreWCF/CoreWCF/blob/f7113a7f2537ecd4d5ca397fbebbbb97aad241a1/src/CoreWCF.Primitives/src/CoreWCF/Runtime/InternalBufferManager.cs

@somdoron, any thoughts on this?

@codecov
Copy link

codecov bot commented May 10, 2022

Codecov Report

Merging #1017 (444a1de) into master (dc15b2d) will increase coverage by 0.04%.
The diff coverage is 0.00%.

❗ Current head 444a1de differs from pull request most recent head ceef6b9. Consider uploading reports for the commit ceef6b9 to get more accurate results

@@            Coverage Diff             @@
##           master    #1017      +/-   ##
==========================================
+ Coverage   65.76%   65.81%   +0.04%     
==========================================
  Files         146      146              
  Lines        9065     9076      +11     
  Branches     1450     1450              
==========================================
+ Hits         5962     5973      +11     
  Misses       2503     2503              
  Partials      600      600              
Impacted Files Coverage Δ
src/NetMQ/BufferPool.cs 34.14% <0.00%> (-12.53%) ⬇️
src/NetMQ/ReceiveThreadSafeSocketExtensions.cs 26.78% <0.00%> (-12.50%) ⬇️
src/NetMQ/Core/Utils/Poller.cs 75.60% <0.00%> (-3.66%) ⬇️
src/NetMQ/Core/SocketBase.cs 71.37% <0.00%> (+0.53%) ⬆️
src/NetMQ/Core/Transports/StreamEngine.cs 63.51% <0.00%> (+3.40%) ⬆️

Subsitute with reference to System.Buffers.
I expect the BufferMangerBufferPool implementation will go away
but before that happens I wanted the tests for each implementation to
remain accessible. If you define USE_SERVICE_MODEL, it will build the
old pool implementation and use it. And it'll also run tests against the
old and new pool.
@shanecelis
Copy link
Author

In response to the feedback and code coverage report given above, I added tests for both pool implementations, and based on those tests I added a fix to make ArrayBufferPool's behavior conform to BufferManagerBufferPool's.

I removed the Flavor build parameter, which simplifies the build back to what it was.

I removed the reference to System.ServiceModel.Primitives and added an explicit reference to System.Buffers even though it's implicitly required by NaCl.Net

I did add a USE_SERVICE_MODEL define so that we can exercise tests against both implementations for the interim.

On macOS, I couldn't run the full test suite. (Something hangs but I didn't investigate it.) I did exercise ArrayBufferPoolTests and BufferManagerBufferPoolTests that I wrote and they pass.

From the testing I did notice one peculiarity. No matter how small the pool size parameters are, one can rent an array much larger than the max array size. This is true with the original implementation and the new implementation preserves this behavior but it seems odd. I would expect an exception to be thrown if I requested an array size that was bigger than my pool's max array size. But that doesn't happen here currently. See BufferPoolTests.cs to see for yourselves.

@shanecelis
Copy link
Author

Is this failure genuinely a result of my changes? I'm having trouble running the test suite on macOS. I'll try on Windows another time.

@shanecelis shanecelis changed the title Add build option to exclude System.ServiceModel.Primitives dependency. Remove System.ServiceModel.Primitives dependency. May 18, 2022
@dxdjgl
Copy link
Contributor

dxdjgl commented Feb 13, 2023

Please be aware that the ArrayBufferPool behaves very differently than the existing BufferManagerBufferPool, very different values needs to be passed for the construction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants