.NET stream capabilities - is the CanXXX test safe?

Question

There is a fairly common pattern used in .NET to test for the capabilities of a class. Here I'll use the Stream class as an example, but the issue applies to all classes that use this pattern.

The pattern is to supply a boolean property called CanXXX to indicate that capability XXX is available on the class. For example, the Stream class has CanRead, CanWrite and CanSeek properties to indicate that the Read, Write and Seek methods can be called. If the properties value is false, then calling the respective method will result in a NotSupportedException being thrown.

From the MSDN documentation on the stream class:

Depending on the underlying data source or repository, streams might
support only some of these capabilities. An application can query a stream for its capabilities by using the CanRead, CanWrite, and CanSeek properties.

And doco for the CanRead property:

When overridden in a derived class, gets a value indicating whether the current stream supports reading.

If a class derived from Stream does not support reading, calls to the Read, ReadByte, and BeginRead methods throw a NotSupportedException.

I see a lot of code written along the lines of the following:

if( stream.CanRead )
{
    stream.Read( ... )
}

Note that there is no synchronisation code, say, to lock the stream object in any manner - other threads may be accessing it or objects that it references. There is also no code to catch a NotSupportedException.

The MSDN documentation does not state that the property value can not change over time. In fact, the CanSeek property changes to false when the stream is closed, demonstrating the dynamic nature of these properties. As such, there is no contractual guarantee that call to Read() in the above code snippet will not throw a NotSupportedException.

I expect that there is a lot of code out there that suffers from this potential problem. I wonder how those who have identified this issue have addressed it. What design patterns are appropriate here?

I'd also appreciate comments on the validity of this pattern (the CanXXX, XXX() pairs). To me, at least in the case of the Stream class, this represents a class/interface that is trying to do too much and should be split into more fundamental pieces. The lack of a tight, documented contract makes testing impossible and implementation even harder!

Answer 1

stream.CanRead just checks whether underlying stream has possibility of reading. It does not say anything about whether actual reading will be possible (e.g. disk error).

There is no need to catch NotImplementedException if you used any of *Reader classes since they all support reading. Only *Writer will have CanRead=False and throw that exception. If you are aware that stream supports reading (e.g. you used StreamReader), IMHO there is no need to make additional check.

You still need to catch exceptions since any error during read will throw them (e.g. disk error).

Also notice that any code that is not documented as thread-safe is not thread-safe. Usually static members are thread safe, but instance members aren't - however, there is need to check documentation for each class.

Answer 2

This sounds more like a theoretical problem than a practical one. I can't really think of any situations in which a stream would become unreadable/unwritable other than due to it being closed.

There may well be corner cases, but I wouldn't expect them to show up often at all. I don't think the vast majority of code needs to worry about this.

It's an interesting philosophical problem though.

EDIT: Addressing the question of whether or not CanRead etc are useful, I believe they still are - mostly for argument validation. For example, just because a method takes a stream which it's going to want to read at some point doesn't mean it wants to read it right at the start of the method, but that's where the argument validation should ideally be performed. This is really no different to checking whether a parameter is null and throwing ArgumentNullException instead of waiting for a NullReferenceException to be thrown when you first happen to dereference it.

Also, CanSeek is slightly different: in some cases your code may well cope with both seekable and non-seekable streams, but with more efficiency in the seekable case.

This does rely on the "seekability" etc remaining consistent - but as I've said, that appears to be true in real life.

---

Okay, let's try putting this another way...

Unless you're reading/seeking within memory and you've already made sure there's enough data, or you're writing within a preallocated buffer, there's always a chance things will go wrong. Disks fail or fill up, networks collapse etc. These things do happen in real life, so you always need to code in a way which will survive failure (or consciously choose to ignore the problem when it doesn't really matter).

If your code can do the right thing in the case of a disk failure, chances are it can survive a FileStream turning from writable to non-writable.

If Stream did have firm contracts, they'd have to be incredibly weak - you couldn't use static checking to prove that your code will always work. The best you could do is to prove that it did the right thing in the face of failure.

I don't believe Stream is going to change any time soon. While I certainly accept that it could be better documented, I don't accept the idea that it is "completely broken." It would be more broken if we couldn't actually use it in real life... and if it could be more broken than it is now, it's logically not completely broken.

I have far bigger issues with the framework, such as the relatively poor state of date/time APIs. They've become a lot better in the last couple of versions, but they're still missing a lot of the functionality of (say) Joda Time. The lack of built-in immutable collections, poor support for immutability in the language etc - these are real problems which cause me actual headaches. I'd rather see them addressed than spend ages on Stream which seems to me to be a somewhat intractable theoretical problem which causes few issues in real life.

Answer 3

From your question and all the subsequent commentary, I'm guessing that your issue is with the clarity and "correctness" of the stated contract. The stated contract being what is in the MSDN online documentation.

What you've pointed out is that there is something missing from the documentation which forces one to make assumptions about the contract. More specifically, because there is nothing said about the volatility of the readability property of a stream, the only assumption that one can make is that it is possible for a NotSupportedException to be thrown, regardless of what the value of the corresponding CanRead property was a few milliseconds (or more) prior.

I think that one needs to go on the intent of this interface in this case, that is:

1. If you use more than one thread, all bets are off;
2. until you go calling something on the interface that will potentially change the state of the stream, you can safely assume that the value of CanRead is invariant.

Notwithstanding the above, Read* methods may potentially throw a NotSupportedException.

The same argument can be applied to all the other Can* properties.

Answer 4

Without knowing the internals of an object, you must assume that a "flag" property is too volatile to rely on when the object is being modified in multiple threads.

I have seen this question more commonly asked about read-only collections than streams, but I feel it's another example of the same design patter, and the same arguments apply.

To clarify, the ICollection interface in .NET has the property IsReadOnly, which is intended to be used as an indicator of whether the collection supports methods to modify its contents. Just like streams, this property can change at any time and will cause InvalidOperationException or NotSupportedException to be thrown.

The discussions around this usually boil down to:

• Why isn't there an IReadOnlyCollection interface instead?
• Whether NotSupportedException is a good idea.
• The pros and cons of having "modes" versus distinct concrete functionality.

Modes are rarely a good thing, as you are forced to deal with more than one "set" of behaviour; having something which can switch modes at any time is considerably worse, as your application now has to deal with more than one "set" of behaviour too. However, just because it's possible to break something down into more discreet functionality does not necessarily mean you always should, particularly when breaking it apart does nothing to reduce the complexity of the task at hand.

My personal opinion is that you have to pick the pattern which is closest to the mental model you perceive the consumers of your class will understand. If you are the only consumer, pick whichever model you like most. In the case of Stream and ICollection, I think that having a single definition of these is much closer to the mental model built up by years of development in similar systems. When you talk about streams, you talk about file streams and memory streams, not whether they're readable or writeable. Similarly, when you talk about collections, you rarely refer to them in terms of "writeability".

My rule of thumb on this one: Always look for a way to break down the behaviours into more specific interfaces, rather than having "modes" of operation, as long as it compliments a simple mental model. If it's hard to think of the separate behaviours as separate things, use a mode-based pattern and document it very clearly.

Answer 5

Okay, here's another attempt which will hopefully be more useful than my other answer...

It's unfortunate that MSDN doesn't give any specific guarantees about how CanRead/CanWrite/CanSeek may change over time. I think it would be reasonable to assume that if a stream is readable it will continue to be readable until it is closed - and the same will hold for the other properties

In some cases I think it would be reasonable for a stream to become seekable later - for instance, it might buffer everything it reads until it reaches the end of the underlying data, and then allow seeking within it afterwards to let clients reread the data. I think it would be reasonable for an adapter to ignore that possibility, however.

This should take care of all but the most pathological cases. (Streams pretty much designed to cause havoc!) Adding these requirements to the existing documentation is a theoretically breaking change, even though I suspect that 99.9% of implementations will obey it already. Still, it might be worth suggesting on Connect.

Now, as for the discussion between whether to use a "capability-based" API (like Stream) and an interface-based one... the fundamental problem I see is that .NET doesn't provide the ability to specify that a variable has to be a reference to an implementation of more than one interface. For example, I can't write:

public static Foo ReadFoo(IReadable & ISeekable stream)
{
}

If it did allow this, it might be reasonable - but without that, you end up with an explosion of potential interfaces:

IReadable
IWritable
ISeekable
IReadWritable
IReadSeekable
IWriteSeekable
IReadWriteSeekable

I think that's messier than the current situation - although I think I would support the idea of just IReadable and IWritable in addition to the existing Stream class. That would make it easier for clients to declaratively express what they need.

With Code Contracts, APIs can declare what they provide and what they require, admittedly:

public Stream OpenForReading(string name)
{
    Contract.Ensures(Contract.Result<Stream>().CanRead);

    ...
}

public void ReadFrom(Stream stream)
{
    Contract.Requires(stream.CanRead);

    ...
}

I don't know how much the static checker can help with that - or how it copes with the fact that streams do become unreadable/unwritable when they're closed.