23 March 2013

Customizing the Ninject kernel configuration for use with WCF proxies

On several of our projects, we are happily using Ninject for our DI needs. Some service implementations we wrote actually call other services using WCF proxies created through an IChannelFactory. Those proxies are injected as dependencies by Ninject where required, and their lifetime is controlled using the InRequestScope() extension for ASP.NET MVC controllers or WCF services.

However, one thing that worried me was the well-known fact that Dispose() cannot be called on proxies without paying some attention to exceptions (see here). Unfortunately (and quite normally), the Ninject StandardKernel is not aware of the gory details of cleaning up correctly WCF objects implementing ICommunicationObject, like proxies. Indeed, as proxies also implement IDisposable, they are by default taken care of by the DisposableStrategy of Ninject, which does a simple call to Dispose() when the object’s lifetime is over.

Fortunately, the Ninject architecture is highly modular and it is very easy to add, remove or replace components in the kernel. A Ninject kernel implements the IKernel interface, and Ninject provides an abstract base class and a full implementation (the StandardKernel).

A kernel internally uses a set of components that provide implementations of various Ninject interfaces it needs. The actual implementations to be used are chosen in the AddComponents() method shown above. The kind of components that take care of object setup and teardown are the ones that implement the IActivationStrategy interface, like the DisposableStrategy, shown below.

The StandardKernel uses a set of activation strategies that are brought into it in its AddComponents() method:

Given this design, it is very easy to replace the DisposableStrategy by a new one that is aware of ICommunicationObject behavior:

We then derive from StandardKernel (or from KernelBase) to use the new activation strategy:

Et voilĂ ! No more nasty CommunicationObjectFaultedException when trying to Dispose of a faulted channel. By the way, this is also how we can remove things we do not need from the StandardKernel. In our case, we know we are going to use exclusively construction injection (no method or property injection), and we will not need support for using one of the Ninject object lifecycle interfaces (IStartable, IInitializable). Therefore we removed the components (activation and planning strategies) dealing with these things from our custom kernel. This is also a nice way to enforce those design decisions (constructor injection only, no dependency on Ninject lifecycle interfaces), just by simply removing the features from the container itself.

As a conclusion, I must say that I really like Ninject. Its clear design makes it easy to use, extend, and customize. It maybe consumes a few more CPU cycles than other DI containers, but it does its job very well, and is an ideal choice to introduce DI in a team that is not familiar with the concept.

28 February 2013

Copying files or streams using asynchronous non-blocking I/O

The Context

I've been intensively working on BizTalk messaging solutions for almost two years now, and one of the operational feature of the platform we put in place is the capture of a copy of all incoming and outgoing messages.
For messages of a reasonable size (say 512 kB once compressed and base64-encoded), we're using the integrated Business Activity Monitoring (BAM) API of BizTalk, which is quite powerful and performs extremely well. The message data is then directly inserted by the BAM infrastructure in the appropriate database tables.
But when message size grows over that limit, we have to switch to another solution, as the data size that can be handled by the BAM is limited to 512kB. In that case we have a pipeline component, that writes the message data as a file on the local file system of the machine where the message is being received or sent. As a side note, this is implemented using the transactional file system support (TxF) and inspired by this post on Paolo Salvatori's blog.
Once the message copy is saved on the local file system, it cannot stay there forever though. BizTalk machines are meant to be 100% stateless, and are not backed up, nor is there plenty of disk space on these machines. Therefore, an agent running as a windows service is continuously moving the message copies towards a central location (share) on a SAN, where it will be accessible for consultation, and later be purged.

It was while I was designing the code for the agent that I realized that it should barely do anything else than I/O.  Since that agent is running in a demanding server environment, I wanted to avoid all unneeded resource consumption, and also maximize throughput. To meet that objective, and following the advice from Jeffrey Richter in his excellent book CLR via C# (3rd edition), I decided that all I/O operations should be done asynchronously in order to avoid blocking any worker thread. Indeed, blocking threads is bad for scalability and forces the thread pool to create more threads than necessary, which consumes more resources (memory, OS thread switching and scheduling, etc.).

The Code


So the whole idea is to avoid blocking any thread at any moment while moving data from one stream to another. Note that this excludes any ThreadPool.QueueUserWorkItem approach where the copy would simply be executed synchronously, but on another thread that would itself block. The solution has to be built around the use of the Begin/EndRead and Begin/EndWrite methods of the Stream class.

A few words of explanation about the AsyncStreamCopier class:

  • it is designed to process reads and writes asynchronously and concurrently. It does this by reusing a series of buffers in a cyclic way. A read operation is started when the next buffer in the cycle has already been written. 
  • a significant part of the code is dedicated to exception handling, and ensuring that any exception that occurs on an I/O thread does not go unnoticed by the caller. In case an exception occurs and is not consumed by the caller, it is thrown either in Dispose, or in the finalizer (process crash guaranteed...).
  • the class implements the APM (Asynchronous Programming Model), and exposes the two methods Begin/EndCopy, so its use should be straightforward: just construct a new instance, passing the source and target streams, and a few other options, then kick off the copy using BeginCopy providing your callback for when the operation finishes. After the copy, do not forget to Dispose() of the instance.
  • the most tricky parts of the code are in the EndRead and EndWrite methods. Both methods can launch the next read and/or write, depending on the current state of the buffers in the cyclic buffer array.
  • all the code uses user-mode constructs (Interlocked methods) for thread synchronization. The only place where a kernel-mode thread synchronization construct is used is the IAsyncResult implementation, to satisfy the APM.
So, here's the code, with a few (hopefully useful) embedded comments.
public class AsyncStreamCopier : IDisposable
{
    // size in bytes of the buffers in the buffer pool
    private const int DefaultBufferSize = 4096;
    private readonly int _bufferSize;
    // number of buffers in the pool
    private const int DefaultBufferCount = 4;
    private readonly int _bufferCount;

    // indexes of the next buffer to read into/write from
    private int _nextReadBuffer = -1;
    private int _nextWriteBuffer = -1;

    // the buffer pool, implemented as an array, and used in a cyclic way
    private readonly byte[][] _buffers;
    // sizes in bytes of the available (read) data in the buffers
    private readonly int[] _sizes;
    // the streams...
    private Stream _source;
    private Stream _target;
    private readonly bool _closeStreamsOnEnd;

    // number of buffers that are ready to be written
    private int _buffersToWrite;
    // flag indicating that there is still a read operation to be scheduled
    // (source end of stream not reached)
    private volatile bool _moreDataToRead;
    // the result of the whole operation, returned by BeginCopy()
    private AsyncResult _asyncResult;
    // any exception that occurs during an async operation
    // stored here for rethrow
    private IOException _exception;

    public AsyncStreamCopier(Stream source,
                             Stream target,
                             bool closeStreamsOnEnd, 
                             int bufferSize = DefaultBufferSize, 
                             int bufferCount = DefaultBufferCount)
    {
        if (source == null)
            throw new ArgumentNullException("source");
        if (target == null)
            throw new ArgumentNullException("target");
        if (!source.CanRead)
            throw new ArgumentException("Cannot copy from a non-readable stream.");
        if (!target.CanWrite)
            throw new ArgumentException("Cannot copy to a non-writable stream.");
        _source = source;
        _target = target;
        _moreDataToRead = true;
        _closeStreamsOnEnd = closeStreamsOnEnd;
        _bufferSize = bufferSize;
        _bufferCount = bufferCount;
        _buffers = new byte[_bufferCount][];
        _sizes = new int[_bufferCount];
    }

    ~AsyncStreamCopier()
    {
        // ensure any exception cannot be ignored
        ThrowExceptionIfNeeded();
    }

    public void Dispose()
    {
        if (_asyncResult != null)
            _asyncResult.Dispose();
        if (_closeStreamsOnEnd)
        {
            if (_source != null)
            {
                _source.Dispose();
                _source = null;
            }
            if (_target != null)
            {
                _target.Dispose();
                _target = null;
            }
        }
        GC.SuppressFinalize(this);
        ThrowExceptionIfNeeded();
    }

    public IAsyncResult BeginCopy(AsyncCallback callback, object state)
    {
        // avoid concurrent start of the copy on separate threads
        if (Interlocked.CompareExchange(ref _asyncResult, new AsyncResult(callback, state), null) != null)
            throw new InvalidOperationException("A copy operation has already been started on this object.");
        // allocate buffers
        for (int i = 0; i < _bufferCount; i++)
            _buffers[i] = new byte[_bufferSize];

        // we pass false to BeginRead() to avoid completing the async result
        // immediately which would result in invoking the callback
        // when the method fails synchronously
        BeginRead(false);
        // throw exception synchronously if there is one
        ThrowExceptionIfNeeded();
        return _asyncResult;
    }

    public void EndCopy(IAsyncResult ar)
    {
        if (ar != _asyncResult)
            throw new InvalidOperationException("Invalid IAsyncResult object.");

        if (!_asyncResult.IsCompleted)
            _asyncResult.AsyncWaitHandle.WaitOne();

        if (_closeStreamsOnEnd)
        {
            _source.Close();
            _source = null;
            _target.Close();
            _target = null;
        }

        ThrowExceptionIfNeeded();
    }

    /// <summary>
    /// Here we'll throw a pending exception if there is one, 
    /// and remove it from our instance, so we know it has been consumed.
    /// </summary>
    private void ThrowExceptionIfNeeded()
    {
        if (_exception != null)
        {
            var exception = _exception;
            _exception = null;
            throw exception;
        }
    }

    private void BeginRead(bool completeOnError = true)
    {
        if (!_moreDataToRead)
        {
            return;
        }
        if (_asyncResult.IsCompleted)
            return;
        int bufferIndex = Interlocked.Increment(ref _nextReadBuffer) % _bufferCount;
        try
        {
            _source.BeginRead(_buffers[bufferIndex], 0, _bufferSize, EndRead, bufferIndex);
        }
        catch (IOException exception)
        {
            _exception = exception;
            if (completeOnError)
                _asyncResult.Complete(false);
        }
    }

    private void BeginWrite()
    {
        if (_asyncResult.IsCompleted)
            return;
        // this method can actually be called concurrently!!
        // indeed, let's say we call a BeginWrite, and the thread gets interrupted 
        // just after making the IO request.
        // At that moment, the thread is still in the method. And then the IO request
        // ends (extremely fast io, or caching...), EndWrite gets called
        // on another thread, and calls BeginWrite again! There we have it!
        // That is the reason why an Interlocked is needed here.
        int bufferIndex = Interlocked.Increment(ref _nextWriteBuffer) % _bufferCount;
        try
        {
            _target.BeginWrite(_buffers[bufferIndex], 0, _sizes[bufferIndex], EndWrite, null);
        }
        catch (IOException exception)
        {
            _exception = exception;
            _asyncResult.Complete(false);
        }
    }

    private void EndRead(IAsyncResult ar)
    {
        try
        {
            int read = _source.EndRead(ar);
            _moreDataToRead = read > 0;
            var bufferIndex = (int) ar.AsyncState;
            _sizes[bufferIndex] = read;
        }
        catch (IOException exception)
        {
            _exception = exception;
            _asyncResult.Complete(false);
            return;
        }

        if (_moreDataToRead)
        {
            int usedBuffers = Interlocked.Increment(ref _buffersToWrite);
            // if we incremented from zero to one, then it means we just 
            // added the single buffer to write, so a writer could not 
            // be busy, and we have to schedule one.
            if (usedBuffers == 1)
                BeginWrite();
            // test if there is at least a free buffer, and schedule
            // a read, as we have read some data
            if (usedBuffers < _bufferCount)
                BeginRead();
        }
        else
        {
            // we did not add a buffer, because no data was read, and 
            // there is no buffer left to write so this is the end...
            if (Thread.VolatileRead(ref _buffersToWrite) == 0)
            {
                _asyncResult.Complete(false);
            }
        }
    }

    private void EndWrite(IAsyncResult ar)
    {
        try
        {
            _target.EndWrite(ar);
        }
        catch (IOException exception)
        {
            _exception = exception;
            _asyncResult.Complete(false);
            return;
        }

        int buffersLeftToWrite = Interlocked.Decrement(ref _buffersToWrite);
        // no reader could be active if all buffers were full of data waiting to be written
        bool noReaderIsBusy = buffersLeftToWrite == _bufferCount - 1;
        // note that it is possible that both a reader and
        // a writer see the end of the copy and call Complete
        // on the _asyncResult object. That race condition is handled by
        // Complete that ensures it is only executed fully once.
        if (!_moreDataToRead)
        {
            // at this point we know no reader can schedule a read or write
            if (Thread.VolatileRead(ref _buffersToWrite) == 0)
            {
                // nothing left to write, so it is the end
                _asyncResult.Complete(false);
                return;
            }
        }
        else
            // here, we know we have something left to read, 
            // so schedule a read if no read is busy
            if (noReaderIsBusy)
                BeginRead();

        // also schedule a write if we are sure we did not write the last buffer
        // note that if buffersLeftToWrite is zero and a reader has put another
        // buffer to write between the time we decremented _buffersToWrite 
        // and now, that reader will also schedule another write,
        // as it will increment _buffersToWrite from zero to one
        if (buffersLeftToWrite > 0)
            BeginWrite();
    }
}

Some usage notes


A simple usage example looks like the following:

private AsyncStreamCopier _copier;
public void DoIt()
{
    var source = new FileStream(@"D:\Temp\SomeBigFile.dat", 
                    FileMode.Open, 
                    FileAccess.Read, 
                    FileShare.None, 
                    8192, 
                    FileOptions.Asynchronous);
    var target = new FileStream(...);

    _copier = new AsyncStreamCopier(source, target, false, 32768, 3);
    _copier.BeginCopy(CopyFinished, null);
}

private void CopyFinished(IAsyncResult ar)
{
    try
    {
        _copier.EndCopy(ar);
    }
    catch (Exception e)
    {
        Console.WriteLine(e);
    }
}

As you can see, the code is relatively simple. Worth noticing, is that FileOptions.Asynchronous must be specified to deal with FileStreams asynchronously. As a general rule, the source and target streams must of course support asynchronous operations. You can play with both different buffer sizes, and different numbers of buffers, and see what leads to the best performance/shortest copy time. A degenerate case is when bufferCount = 1. In that case, concurrent reads and writes are disabled, as there is only a single buffer to read into and write from. That is (intuitively) probably the configuration to use when both streams use the same underlying physical device (same disk controller, same network card...), as otherwise both operations will compete for the device. In the context described here above, I could safely use concurrency for reads and writes, as the source was a file on a local disk, and the destination was located on a network share.
As a final note, you can find in Stephen Toub's .NET Matters column another (simpler) implementation for dealing with streams asynchronously. The main difference with the solution described here above is that concurrent reads and writes are not supported by Stephen's code. By the way, I'm curious if the code would be significantly simpler using the new C# 5 async support.

You can download the code here.

02 February 2009

The Daily WTF

Here's an excerpt from a dialog I recently had within the company I'm currently working for:

Me: Hi! I'm Yves and I've been assigned as software architect for project X (note: project is already running for 2 months…). What are you doing on the project?

Guy: I'm Georges and I'm technical analyst.

Me: I'll perform a code review on Wednesday with the development team. Will you be there?

Guy: Oh no, surely not! I can't write a single line of code!

He meant it. That would deserve being published on The Daily WTF.

29 November 2008

aspnet_compiler and missing .compiled files


This week, I've been facing a rather annoying issue. A developer told me that some global.asax events no longer fired after he precompiled his ASP.NET application using aspnet_compiler on his computer. He noticed that this behavior was due to a missing App_global_asax.dll.compiled file in the output bin directory of the precompiled application. Strangely, when he just published the web site using the Visual Studio publish web site command, everything went well. When aspnet_compiler was invoked through our NAnt build script, .compiled files were missing.

I spent a few hours on this one…I discovered that this behavior was reproducible on my computer too, but not consistently. Starting aspnet_compiler directly from a command prompt, or invoking MSBuild on the solution file containing the web site did sometimes produce the required .compiled files, but rarely. This was driving me crazy. How could a compiler not be consistent, "forget" about some files, without even producing any kind of errors? I monitored the entire compilation process using Process Monitor, and I did not notice any suspect. When everything went well, aspnet_compiler just created and copied the .compiled files, otherwise, it seemed to simply not even attempt to do it. No access denied whatsoever, not a single file system error…After a few hours, a colleague and I had a stupid idea (well, it appeared to us that it was not so stupid): we killed all the antivirus processes using task manager. And guess what? All of a sudden, aspnet_compiler systematically created the required .compiled files, at every build and with whatever method we used to launch it (VS publish, MSBuild, NAnt, command-line)…We did the same test on the developer's computer (which of course ran the same antivirus), and it worked like a charm.

Lesson learned: antivirus programs are not the developer's friends.

We already experienced some annoying AppDomain restarts while debugging ASP.NET applications, due to the antivirus touching web.config and other monitored files. And now, it interferes even with a compiler, and this without causing any actual build failure! I admit that I do not understand what really happens at the filesystem level: why do I not see anything in Process Monitor (including no antivirus activity on .compiled files)? Do these antivirus programs sit even lower in the OS architecture than Process Monitor so that they can "swallow" events? Maybe…Anyway, whenever I'll experience some strange behavior of my development tools in the future, one of the first thing I'll do is turn off the antivirus. And, by the way, I'm very happy that no AV is running on our build server, otherwise…

Last thing: our machines were running the CA eTrust antivirus. On my second laptop, where I have the free edition of AVG, I never encountered any issue of this kind.

24 September 2008

Are you certified?

Today, I just passed the Microsoft exam 70-536 App Development Foundation. I succeeded with a fairly reasonable score, but I was a little bit disappointed, so to say. It was back in 2004 when I passed such an exam for the last time and I had hoped that the actual skills assessed by the exam would better reflect someone's proficiency in a software engineering job. Unfortunately, the questions I was asked were still in the same style as 4 years ago…The key skill you must possess to pass the exam is that you must know the API of the BCL more or less by heart! What a funny thing! This is not all what I expect from a decent software engineer. Such an exam should somehow measure the real understanding the candidate has of the platform, and its ability to apply this understanding to solve new problems. If I don't know the exact syntax and parameter order/signification for a method, I have these little things called Intellisense, MSDN help and F1 in my IDE, no?

Anyway, I'm going to continue my certification path towards the MCPD: Enterprise Developer, and see if the next exams on WCF, ASP.NET and Windows Forms are like this one. Frankly, I don't expect much difference. But at the end, I'll have my "marketing" certification, at least. Because the value I see in such a certification from a technical viewpoint is, well, close to zero…The only Microsoft certification that I think would really reflect that you are indeed very smart and skilled in your job is the Microsoft Certified Architect program. But that's a 10000$ story…

31 July 2008

It’s all about coupling…

When designing a framework or reusable components, a basic design principle is to keep an eye on the component's dependencies. These dependencies towards other components should always be minimized. Having external dependencies will force the user of the component to depend on these as well, which is generally not desirable for many reasons: sensitivity to change, release cycles and versioning, conflicts with other components… This is the well-known high cohesion/loose coupling story.

I was recently (today) confronted with a component whose design does not quite follow that principle: the Enterprise Library Exception Handling Application Block. While I find that the component does what it has to do rather well, I don't appreciate at all its dependencies. My idea was to write an exception handling aspect using PostSharp and the Exception Handling block. Rather simple, no? Well, no, not so simple, as I have a number of constraints: my architecture uses Dependency Injection, and I don't want any instrumentation for now to name two. These constraints have the effect to make the block in its current design useless for me, as it has direct dependencies on ObjectBuilder2 and Unity and instrumentation primitives. In other words, it is tightly coupled to its surrounding runtime environment. Of course, my DI container is not Unity (I use Spring.NET), and I do not want in any way to depend on it!

I think that when you just want to implement a consistent exception handling across an application (using a component that handles this task well), that should not force you to use DI, and certainly not a specific DI container. This goes against some good design principles.

What I could admit is a dependency towards an abstract assembly that expresses the need of the block for a DI infrastructure (e.g. through attributes), but not that the block depends directly on a particular implementation of such an important infrastructure component as a DI container. Now, I'm stuck with two possibilities: modify the block code to extract the depending code in its own separate assembly (let's hope it is well isolated, I'll have to check this), or redevelop my own block implementation (sad because I like how the core concern of the block is handled…).

And by the way, how do I get rid of those instrumentation calls? It's not that I do not recognize there is value in that stuff, but I do not need it in a (unit) test environment, and I don't want to install any perf counters or configure WMI on my laptop just to handle exceptions. In previous versions of the Enterprise Library, we could recompile it and exclude the instrumentation using conditional compilation, but this does not seem to be possible any more.

Conclusion: always actively manage dependencies (minimize them), and when designing a framework or a reusable component, always think that the user must only pay for what he actually uses.

I would be curious to let NDepend run on the whole Enterprise Library and see what the results are. Maybe if I have spare time ;-)…

12 February 2008

About IDisposable, Close(), Streams and StreamReader-Writer

Last week, we discovered a bug in a SOAP extension our team wrote. An ObjectDisposedException occurred during the extension invocation on the client side.

The call stack ended like this:

System.ObjectDisposedException: Cannot access a closed Stream.


at System.IO.__Error.StreamIsClosed()
at System.IO.MemoryStream.set_Position(Int64 value)
…(upper in call chain)

Oops, we're trying to use a closed Stream…How can that be? Well, the code that actually uses the Stream is the following:


public void ProcessMessage()
{
// ...
CopyStream(_oldStream, _newStream);
_newStream.Position = 0;// <= this is where the exception occurs
// ...
}

private static void CopyStream(Stream source, Stream target)
{
using (TextReader reader = new StreamReader(source))
using (TextWriter writer = new StreamWriter(target))
{
writer.Write(reader.ReadToEnd());
}
}


It looks like _newStream is being closed inside CopyStream, and when I look at the method, the only place where this could happen is the Dispose call on the StreamWriter, when leaving the using block. So, it looks like the StreamWriter is effectively becoming the owner of the underlying Stream it uses! Let's look at what really happens in StreamWriter.Dispose, thanks to our friend Reflector.


public abstract class TextWriter : MarshalByRefObject, IDisposable
{
public virtual void Close()
{
this.Dispose(true);
GC.SuppressFinalize(this);
}

public void Dispose()
{
this.Dispose(true);
GC.SuppressFinalize(this);
}

protected virtual void Dispose(bool disposing)
{
}

// Other members...
}

public class StreamWriter : TextWriter
{
private Stream stream;
private bool closable;

internal bool Closable
{
get
{
return this.closable;
}
}

public override void Close()
{
this.Dispose(true);
GC.SuppressFinalize(this);
}

protected override void Dispose(bool disposing)
{
try
{
// some code omitted here...
}
finally
{
if (this.Closable && (this.stream != null))
{
try
{
if (disposing)
{
this.stream.Close();
}
}
finally
{
this.stream = null;
// setting other members to null here…
base.Dispose(disposing);
}
}
}
}
}

Ah aah, OK, I understand. Indeed when disposing of the StreamWriter, it calls Stream.Close. But I noticed that there is this Closable property, that when set to true, could avoid the closing of the Stream. Unfortunately, it's internal, and it happens to be used by the framework only (for Console streams apparently). Frankly, I don't like this design. I think it would have been better to make Stream ownership by the StreamWriter optional, and that's what the Closable property seems to have as purpose, so why isn't there a way to set it? The only StreamWriter constructor that allows setting it is also internal. Moreover, why does TextWriter.Dispose call GC.SuppressFinalize(this) while there is no finalizer defined in the class? And there is also the override of the Close method which just…repeats the base implementation (I'll discuss this implementation further, together with Stream).

Hmm, I'm not happy with what I discovered in the framework, but maybe I would not have had this bug if I had read the doc for the StreamWriter constructor? So I read it, and guess what? It is nowhere mentioned that the lifetime of my Stream object is now tightly coupled to the StreamWriter. Not a single word about it. But there's a nice sample of code illustrating the usage of the different constructors of StreamWriter. Here it is:


public void CreateTextFile(string fileName, string textToAdd)
{
string logFile = DateTime.Now.ToShortDateString()
.Replace(@"/", @"-").Replace(@"\", @"-") + ".log";

FileStream fs = new FileStream(fileName,
FileMode.CreateNew, FileAccess.Write, FileShare.None);

StreamWriter swFromFile = new StreamWriter(logFile);
swFromFile.Write(textToAdd);
swFromFile.Flush();
swFromFile.Close();

StreamWriter swFromFileStream = new StreamWriter(fs); // <= look at this
swFromFileStream.Write(textToAdd);
swFromFileStream.Flush();
swFromFileStream.Close(); // <= and also at this…

StreamWriter swFromFileStreamDefaultEnc =
new System.IO.StreamWriter(fs, // <= and finally at this…
System.Text.Encoding.Default);
swFromFileStreamDefaultEnc.Write(textToAdd);
swFromFileStreamDefaultEnc.Flush();
swFromFileStreamDefaultEnc.Close();

// more code omitted…
}

If you try to run the sample, it will of course miserably fail…Because closing the StreamWriter also closes the underlying Stream, so it can't be used again on the next line.

But that's not all…What I also found a little bit strange when looking at the above code, is the fact that it's Stream.Close that's being called by StreamWriter.Dispose, and not Stream.Dispose. Why? Shouldn't owned IDisposable objects be Disposed of during Dispose? Well, it happens that for Stream-derived classes, calling Dispose or Close has strictly the same effect. Let's look at the code:


public abstract class Stream : MarshalByRefObject, IDisposable
{
public void Dispose()
{
this.Close();
}

public virtual void Close()
{
this.Dispose(true);
GC.SuppressFinalize(this);
}

protected virtual void Dispose(bool disposing)
{
if (disposing && (this._asyncActiveEvent != null))
{
this._CloseAsyncActiveEvent(Interlocked.Decrement(ref this._asyncActiveCount));
}
}

// Other members...
}

public class MemoryStream : Stream
{
protected override void Dispose(bool disposing)
{
try
{
if (disposing)
{
this._isOpen = false;
this._writable = false;
this._expandable = false;
}
}
finally
{
base.Dispose(disposing);
}
}

// Other members...
}

So Dispose calls Close, which calls Dispose(true) in the Stream base class, and MemoryStream.Dispose(bool) does nothing interesting but calling the base implementation. Wow, what's that? Again, I don't like that design. Why have a Dispose and a Close when both are doing the same? IMO, I think something is flawed in Stream (and StreamWriter/Reader…). There's a semantic difference between disposing of an object and closing it: in the former case, I don't need it anymore and it's gone, while in the latter, I might to re-open it and use it again. I'm thinking for example of a magazine: when I read it in its entirety, I put it in the recycle bin (that's the Dispose), and when I just read some articles and I plan to read it further later, I close it and leave it on my desk.

To Close or to Dispose?

I think having Close call Dispose brings some confusion. Why have the two? Anyway the contract for a user of a class that implements IDisposable is that it is mandatory to call Dispose after use. Any call to Close is then superfluous. If Close would have allowed me to temporarily release some resource and reuse it later through a call to some Open method, then I think it would make sense. There's another place in the framework 2.0 where this Dispose(),
Dispose(bool), Finalize(), Open(), Close() stuff is IMO almost correctly implemented: System.Data.SqlClient.SqlConnection. I say almost, because trying to re-Open a disposed connection yields an InvalidOperationException and not an ObjectDisposedException, as I would expect. For the rest, it is quite possible to call Open and Close multiple times without any problem.

A simple solution to the Stream ownership issue

The solution I found to the above issue (I'm actually talking about the Stream being closed by the StreamWriter) is rather simple (and I guess I'm not the first one to implement it): use a Decorator around the Stream to prevent it from being closed or disposed. That way, the Stream object is still usable even after the StreamWriter/Reader has been closed.


public class NonClosingStreamDecorator : Stream
{
private readonly Stream _innerStream;

public NonClosingStreamDecorator(Stream stream)
{
if (stream == null)
throw new ArgumentNullException("stream");
_innerStream = stream;
}

public override void Close()
{
// do not delegate !!
//_innerStream.Close();
}

protected override void Dispose(bool disposing)
{
// do not delegate !!
//_innerStream.Dispose();
}

// The rest of the overriden methods simply delegate to InnerStream
}

Using the Decorator is straightforward:


[Test]
public void TestClosingStreamWriterWithNonClosingStream()
{
TextWriter writer = new StreamWriter(new NonClosingStreamDecorator(_stream));
writer.WriteLine("I like this");
writer.Close();
// enjoy using the Stream further
_stream.Position = 0;
}