Is it OK to pass a stream around to multiple methods?

Question

I have an interface defined as:

public interface IClientFileImporter
{
    bool CanImport(Stream stream);
    int Import(Stream stream);
}

The idea is to take any file stream and run it through a series of implementations of this interface to determine which one should handle the file. Some of the implementations may look for a certain header row, while others may look for a certain byte sequence, etc...

My question is, is it OK to pass a stream around like this as long as I never close it? Each method would have to be responsible for resetting the stream to position 0 if necessary, but are there any other potential issues (aside from thread safety)? This code really smells, IMO, but I'm not sure of a better way to do it.

I don't think it's a bad concept in the first place. You don't know what the implementations will need from the stream so providing the whole stream makes sense. To ensure that the implementation doesn't mess with the stream you could implement some kind of wrapper around Stream (deriving from stream itself) that forbids any method that modifies the underlying stream or whatever you need. Additionally, I wouldn't require the implementation to reset the stream position. The caller of CanImport/Import can do it. Combined this ensures that the no importortr can harm the underlying stream. — Andre Loker, Feb 28 '12 at 20:41

score 4 · Accepted Answer · answered Feb 28 '12 at 20:46

4

To prevent the underlying stream from being modified, create a wrapper stream that derives from Stream and forwards only safe calls to the wrapped stream. Also, don't assume the Import/CanImport methods reset the stream position. The caller of those method should reset the stream to a valid state before passing it to Import/CanImport.

answered Feb 28 '12 at 20:46

Andre Loker

8,368
1
23
36

In this read-only stream wrapper, would it be OK to override the Dispose method to automatically rewind the stream but not close it? – Chris Feb 28 '12 at 21:16
Yes, that could certainly work, that way you can use a convenient using block around the calls to CanImport/Import – Andre Loker Feb 28 '12 at 23:17

score 2 · Answer 2 · answered Feb 28 '12 at 20:37

2

If each function returns the stream just the way it got it, I don't think there's a problem with it.

answered Feb 28 '12 at 20:37

zmbq

38,013
14
101
171

Why should the method not modify the stream? I think reading and writing from the stream is the reason you would pass it around. – cadrell0 Feb 28 '12 at 20:41
If it modifies the stream, you can get into all sorts of dependencies between functions. It can be OK, but OP talked about rewinding the stream each time. – zmbq Feb 28 '12 at 20:42

score 2 · Answer 3 · answered Feb 28 '12 at 20:40

2

This shouldn't be a problem.

Although I would probably restructure it slightly:

public interface IClientFileImporter
{
    int Import(Stream stream);
}

Then I would have the Import method return a -1 if it was not able to. Might make your other code a bit simpler.

answered Feb 28 '12 at 20:40

NotMe

87,343
27
171
245

I agree with Chris, nice answer Chris. I think this will make it a bit easier – Dmitry Savy Feb 28 '12 at 20:46
Yeah, return -1 like if it was 1989. While -1 was common in C, it is not common in .NET, a better return value would be a boolean. – Fred Apr 13 '22 at 18:56

score 0 · Answer 4 · answered Feb 28 '12 at 20:46

It is perfectly fine to pass the same stream to multiple methods.

Beware of non-seek-able streams - there are streams that you can't reset position. Andre Loker's comment has good advice to wrap Stream so CanImport methods don't mess the actual stream.

You can also consider explicitly providing some "header" portion of stream to CanImport methods, also it will make them less flexible.

score 0 · Answer 5 · edited Apr 13 '22 at 18:55

If you are worried about passing a stream around because you might end up running external code that might not be trustworthy, then the best you can do is make a new, readonly stream and pass that around so that no external code can change the contents of the file until you are sure you want to let them.

public class ReadOnlyStream : Stream
{
    private Stream _ownerStream;

    public ReadOnlyStream(Stream baseStream)
    {
        _ownerStream = baseStream;
    }
    
    public override bool CanWrite => false;
    
    public override int Write(byte[] bits, int offset, int count)
    {
        throw new InvalidOperationException();
    }
    
    // Other code omitted
}

Is it OK to pass a stream around to multiple methods?

5 Answers5

Linked