.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 documentation 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

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.

Answer 2

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 3

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.