bobvanderlinden / sharpfilesystem

A virtual file system for .NET written in C#
MIT License
282 stars 69 forks source link

Remove “off-topic” System.IO classes (SafeNetworkStream is actually dangerous-looking) #7

Closed binki closed 8 years ago

binki commented 8 years ago

SafeNetworkStream seems to be quite unrelated to the core goal of this project. Also, it places itself in System.IO, meaning it will show up in Intellisense in such a way that the unsuspecting developer might thing it is a Microsoft-provided API. And, even worse, it does things like the following to make stuff “safe”:

try
{
    return _stream.Read(buffer, offset, count);
}
catch (IOException)
{
    return 0;
}
catch (ObjectDisposedException)
{
    return 0;
}

Almost always when an IOException would be thrown, the developer should be handling that in some way. For example, it signals that the connection was reset and that the handle needs to be cleaned up. Code written against streams will expect this behavior to mean that the end of file was reached and it will assume that the bytes read so far represents the entirety of the entity being read. Whereas an IOException is a sign that the connection was interrupted without the sender claiming it was done.

Also, encountering an ObjectDisposedException is the sign of a bug in the code which called Read() on the disposed/closed stream. If the caller can’t even figure out if it closed a Stream handle it has, it’s not keeping track of its state properly. That’s something you’d want thrown so that at least it could be logged or, when developing, crash the app so that you can investigate and fix the bug!

Other classes in the IO folder might actually be useful utilities, but they seem to be wrongly polluting the System.IO namespace and not really related to the purpose of sharpfilesystem. If any of them are actually required for sharpfilesystem to work, it would be better for them to either be marked internal or, if they are to support some of the addon archive assemblies, moved to a namespace that clearly marks them as for sharpfilesystem’s own use, such as namespace SharpFileSystem.Internal.IO.

binki commented 8 years ago

For example, it signals that the connection was reset and that the handle needs to be cleaned up. Code written against streams will expect this behavior to mean that the end of file was reached and it will assume that the bytes read so far represents the entirety of the entity being read.

Here I am meaning to say that when the API returns 0, normal Stream-using code will assume that meant a successful read of the entire entity. Whereas this “safe” wrapper is basically silently truncating the read data.

FrozenCow commented 8 years ago

Yes, totally agree. They were already in the IO directory, so it would be logical to namespace them in SharpFileSystem.IO. I think they could be useful in other projects as well. They are also simple enough to keep supporting them. So I think SharpFileSystem.IO instead of SharpFileSystem.Internal.IO is in order.

SafeNetworkStream was used to normalize Socket-based streams in some hacky applications in the past. For networking read 0 can often be handled the same way as IOException (socket was closed). I agree it is dirty. It also isn't in use by any of SharpFileSystem, so I removed it.