mgravell / Pipelines.Sockets.Unofficial

.NET managed sockets wrapper using the new "Pipelines" API
Other
417 stars 54 forks source link

MemoryMappedPipeReader Feedback #35

Open airbreather opened 5 years ago

airbreather commented 5 years ago

I was playing around with Pipelines.Sockets.Unofficial.MemoryMappedPipeReader this morning, and I'd like to share some feedback from my experience:

  1. Nothing seems to have a finalizer, so those pointers we acquire won't go away unless the consumer explicitly disposes the MemoryMappedPipeReader (ideally, see the remarks and make sure that ReleasePointer is always called in a CER (see CriticalFinalizerObject), though just making sure it's released in a finalizer may be good enough... probably nobody's going to hunt you down if you assume that people won't be regularly aborting threads or unloading app domains).
  2. The return type of MemoryMappedPipeReader.Create does not implement IDisposable, so most people may not even realize that they're supposed to dispose this.
  3. CreateFromFile throws an exception if file.Length is 0... probably just fall back in this case too.
  4. We access the SafeBuffer on UnmanagedMemoryAccessor through reflection (and fall back all the way to the FileStream approach if that's impossible), but wouldn't it be equivalent to go through MemoryMappedViewAccessor.SafeMemoryMappedViewHandle? From my reading of the code, it looks like that would even be the same object instance.