Class for easier to read IProgress<Class> handling (Version 2.0)

Question

This is an update to my question Class for easier to read IProgress handling and includes the suggestions from @svick, with one exception.

It was also suggested to change the setter for Content, so the report object could be set directly. I didn't make this change because this could result in reporting the same instance more than once, resulting in exactly that bug this class was created to avoid.

Preface:

The recommended way to report something as "progress" back from a async function is to use IProgress<Type> or IProgress<Class>.

When the caller invokes the async function, it can provide an implementation for IProgress. This object is then used inside the called instance to report back progress.

For a better description of IProgress, please see Reporting Progress from Async Tasks by Stephen Cleary.

Example code:

Code for the consumer (that wants to receive progress):

//Create progress object
Progress<ProgressDetail> progressRunner = new Progress<ProgressDetail>();
progressRunner.ProgressChanged += RunnerProgressUpdate;

//Pass it to the async function
bool result = await simpleRunner.RunAsync(progressRunner);

Called function (that reports back progress):

public async Task<bool> RunAsync(IProgress<ProgressDetail> progress)
{
  if (progress != null)
  {
     ProgressDetail rp = new ProgressDetail();
     rp.Starting = true;
     progress.Report(rp);
  }
}

Description:

Although the implementation inside the called function is trivial, IMHO it creates too much code for the simple task of reporting back progress. The if check is required because it is perfectly legal that a caller passed in null if no progress is desired.

Beside this, it is also important that the reported object is used only once. Because the object might be handled in an entirely different thread, each call of Report() must yield a new instance.

Quote from Reporting Progress from Async Tasks by Stephen Cleary:

...it means you can’t modify the progress object after it’s passed to Report. It is an error to keep a single “current progress” object, update it, and repeatedly pass it to Report.

Goal:

The goal for this class is:

• Get rid of the NULL check altogether in order to make the code easier to read (I simply like code that can be read from top to down without conditions whenever possible).

• Prevent the reuse of the object that is reported as progress and throw an exception if someone tries it anyway, as this is a typical heisenbug that is hard to debug.

• Allow re-usability of an instance of this class so the usage is more coder friendly

New class

To solve this, the class ProgressReporter was created. Using this class allows the following code:

ProgressReporter<ProgressDetail> _reporter;

public async Task<bool> RunAsync(IProgress<ProgressDetail> progress)
{
    //Assign progress reporter
    _reporter = new ProgressReporter<ProgressDetail>(progress, createNewInstanceAfterReport:true);

   _reporter.Content.Starting = true;
   _reporter.Report();

   //Continue to use _reporter...
   ....
}

• Consumer, Line 207+.
• Async method that uses this class, Line 34+
• The class itself

Final note: I know I ended up smurf naming all the things. Suggestions for better naming are very welcome.

/// <summary>
/// A helper class to report status using the IProgress interface and reporting back a class (TReported).
/// The object beeing reported can used only be ONCE. If createNewInstanceAfterReport is FALSE (the default), calling Report again will raise an exception.
/// If createNewInstanceAfterReport is TRUE, a new instance of the reported object will automatically be created after calling Report().
/// </summary>
/// <typeparam name="TReported">The object beeing reported when calling Report()</typeparam>
public class ProgressReporter<TReported> where TReported : class, new()
{
    IProgress<TReported> _progress;

    bool _createNewInstanceAfterReport;

    /// <summary>
    /// Provides access to the instance that is beeing reported as the "Progress". 
    /// The setter is private ON PURPOSE to avoid that a caller passes in an already used instance which could result in exactly that bug we are trying to avoid with this class.
    /// </summary>
    public TReported Content { get; private set; }


    /// <summary>
    /// Create an instance of this class, requires the IProgress<TReported> implementation that is used to report progress.
    /// </summary>
    /// <param name="progress">IProgress implementation used to report progress</param>
    /// <param name="createNewInstanceAfterReport">TRUE if a new instance of Content should automatically be created after Report()</param>
    public ProgressReporter(IProgress<TReported> progress, bool createNewInstanceAfterReport = false)
    {
        _progress = progress;
        Content = new TReported();

        _createNewInstanceAfterReport = createNewInstanceAfterReport;
    }

    /// <summary>
    /// Reports Content back as progress. Can be used only ONCE if createNewInstanceAfterReport is FALSE.
    /// </summary>
    public void Report()
    {
        //MTH: We need to make sure that an instance of this class is used only once because ReportedObject might be used in a entire different thread. 
        //Quote from [Reporting Progress from Async Tasks](http://blog.stephencleary.com/2012/02/reporting-progress-from-async-tasks.html) by Stephen Cleary
        //"...it means you can’t modify the progress object after it’s passed to Report. It is an error to keep a single “current progress” object, update it, and repeatedly pass it to Report."
        if (Content != null)
        {
            if (_progress != null)
            {
                _progress.Report(Content);
            }

            //Set Content to null no matter if we have _progress or not to make sure that an incorrect use of this class is detected.
            Content = null;

            //If requested by the caller, create a new instance
            if (_createNewInstanceAfterReport)
            {
                Content = new TReported();
            }
        }
        else
        {
            //Seems you didn't read the part about "USE IT ONLY ONCE".
            //Here's an exception for you.
            //You're welcome. 
            throw new InvalidOperationException("An instance of the reported object can only be used once");
        }
    }


}

Answer 1

ProgressReporter.Report can report on the same instance more than once. To see this, let's define a type Foo:

class Foo
{
    private static int _counter;

    private readonly int _id;

    public Foo()
    {
        _id = Interlocked.Increment(ref _counter);
    }

    public int Id
    {
        get { return _id; }
    }
}

And run this code

for (var i = 0; i < 10000; i++)
{
    try
    {
        var progress = new Progress<Foo>();
        progress.ProgressChanged +=
            (sender, e) => Console.WriteLine("Reported {0}", e.Id);
        var reporter = new ProgressReporter<Foo>(progress);
        Parallel.For(0, 2, _ => reporter.Report());
        Console.WriteLine("Reported same object twice");
        break;
    }
    catch (AggregateException)
    {
    }
}

If you run it, you should see output like this

...
Reported 215
Reported 216
Reported 217
Reported 217
Reported same object twice

I think this can be fixed using Interlocked.Exchange

public class ProgressReporter<TReported> where TReported : class, new()
{
    private readonly IProgress<TReported> _progress;

    private readonly bool _createNewInstanceAfterReport;

    private TReported _content;

    public TReported Content
    {
        get { return _content; }
    }

    public ProgressReporter(
        IProgress<TReported> progress,
        bool createNewInstanceAfterReport = false)
    {
        _progress = progress;
        _content = new TReported();
        _createNewInstanceAfterReport = createNewInstanceAfterReport;
    }

    public void Report()
    {
        if (_progress == null)
        {
            return;
        }

        var content = Interlocked.Exchange(
            ref _content,
            _createNewInstanceAfterReport ? new TReported() : null);
        if (content == null)
        {
            throw new InvalidOperationException("An instance of the reported object can only be used once");
        }

        _progress.Report(content);
    }
}

Answer 2

It's absolutely fine to have such a helper. If you want to hear my opinion - I would rather simplify your original code as following:

public async Task<bool> RunAsync(IProgress<ProgressDetail> progress)
{
     progress = progress ?? new Progress<ProgressDetail>();

     ...
     progress.Report(new ProgressDetail { Starting = true } );
  }
}