Open shanecelis opened 2 years ago
Is there a reason not to outright replace the prior implementation? Losing the dependency on a WCF library seems sensible.
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.
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?
Merging #1017 (444a1de) into master (dc15b2d) will increase coverage by
0.04%
. The diff coverage is0.00%
.:exclamation: 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%) |
:arrow_down: |
src/NetMQ/ReceiveThreadSafeSocketExtensions.cs | 26.78% <0.00%> (-12.50%) |
:arrow_down: |
src/NetMQ/Core/Utils/Poller.cs | 75.60% <0.00%> (-3.66%) |
:arrow_down: |
src/NetMQ/Core/SocketBase.cs | 71.37% <0.00%> (+0.53%) |
:arrow_up: |
src/NetMQ/Core/Transports/StreamEngine.cs | 63.51% <0.00%> (+3.40%) |
:arrow_up: |
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.
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.
Please be aware that the ArrayBufferPool behaves very differently than the existing BufferManagerBufferPool, very different values needs to be passed for the construction.
In an effort to use NetMQ with Unity3D, one problem was trying to satisfy the dependencies for
System.ServiceModel.Primitives
. It required also providingSystem.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 theSystem.ServiceModel.Primitives
dependencies.I added an alternative implementation of
IBufferPool
using theArrayPool
fromSystem.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".
This excludes "System.ServiceModel.Primitives" and defines a preprocessor flag of
FLAVOR_MINIMAL
that excludes the BufferManagerBufferPool class and uses the ArrayBufferPool instead.