Today I stumbled across a very subtle pitfall when using the Decorator pattern on an interface that exposes C# events.

Let’s say you have the following interface

1
2
3
4
5
6
7
public interface IDocument
{
    void Close();
    // imagine more members here

    event EventHandler Closed;
}

and, to use the probably most canonical example for decorators, you want to add caching without violating the SRP, so you write the following decorator:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
public class CachingDocumentDecorator : IDocument
{
    readonly IDocument _decoree;
    public CachingDocumentDecorator(IDocument decoree) => _decoree = decoree;

    public void Close() => _decoree.Close();
    // imagine more members being wrapped and forwarded here, some of which do caching

    public event EventHandler Closed
    {
        add => _decoree.Closed += value;
        remove => _decoree.Closed -= value;
    }
}

And, voila!, you have just introduced a subtle bug. Can you spot it?

Imagine another type consuming IDocument:

1
2
3
4
5
6
7
8
9
10
11
public class DocumentBinder
{
    public void Add(IDocument document)
    {
        OpenDocuments.Add(document);
        document.Closed += OnDocumentClosed;
    }

    public ICollection<IDocument> OpenDocuments { get; } = new List<IDocument>();
    void OnDocumentClosed(object sender, EventArgs e) => OpenDocuments.Remove((IDocument) sender);
}

Let’s write a quick test to check that the OpenDocuments property will be kept up-to-date:

1
2
3
4
5
6
7
8
9
10
11
var binder = new DocumentBinder();
var first = Substitute.For<IDocument>();
new[]
{
    first,
    Substitute.For<IDocument>()
}.ForEach(binder.Add);

first.Closed += Raise.Event(); // fire the event        

binder.OpenDocuments.Should().HaveCount(1);

(If the marked line puzzles you, this is just how you simulate an event firing on a mock object created with NSubstitute.)

This test passes.

But what happens if DocumentBinder is used with decorated IDocuments? Let’s adjust our test a tiny bit to see:

1
2
3
4
5
6
7
8
9
10
11
12
var binder = new DocumentBinder();
var first = Substitute.For<IDocument>();
new[]
{
    first,
    Substitute.For<IDocument>()
}.Select(d => new CachingDocumentDecorator(d)) // <-- the change
.ForEach(binder.Add);

first.Closed += Raise.Event();

binder.OpenDocuments.Should().HaveCount(1);     // <-- the test fails here

Now the last line of the test will fail, the actual count of OpenDocuments will be 2.

Why?

Because when the decorated IDocument fires Closed, it passes itself as sender argument. And the decorator does nothing to change sender - so handlers of Closed will always receive undecorated IDocuments as sender. But DocumentBinder adds the decorated documents to OpenDocuments. So when it handles Closed and removes sender from OpenDocuments, it will remove an instance which never has been added to the collection.

Before we fix this issue, let’s write another test for the decorator to more clearly reproduce the problem at its root:

1
2
3
4
5
6
7
8
9
10
11
12
13
var decoree = Substitute.For<IDocument>();
var underTest = new CachingDocumentDecorator(decoree);
// the next line is a FluentAssertions construct 
// that helps us check whether events have been fired
var monitor = underTest.Monitor(); 

decoree.Closed += Raise.Event(); 

var @event = monitor.OccurredEvents.Single();
@event.EventName.Should().Be(nameof(IDocument.Closed));
var sender = @event.Parameters.First();
sender.Should().NotBeSameAs(decoree);       // <-- the test fails here
sender.Should().BeSameAs(underTest);

The fix for this issue is straight-forward: in the decorator, declare the event regularly, attach to the event of _decoree and forward it while changing the sender:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
public class CachingDocumentDecorator : IDocument
{
    readonly IDocument _decoree;

    public CachingDocumentDecorator(IDocument decoree)
    {
        _decoree = decoree;
        _decoree.Closed += OnClosed;
    }

    void OnClosed(object? sender, EventArgs e) => Closed?.Invoke(this, e);

    public void Close() => _decoree.Close();
    // imagine more members being wrapped and forwarded here

    public event EventHandler Closed;
}

Now both failing tests will pass.

As a closing remark, beware that even ReSharper does this wrong: when you use the quick-fix “Delegate implementation of X to new field…”, the code generated by it will look like our first version.