Is this a thread-safe implementation of background bitmap generation?

Question

For a game implementation, I have a very large overview map with multiple layers (namely base-map / units / highlight & info / fov-shading) of which the first is CPU-intensive to generate. I have implemented a caching mechanism in which the base layer is drawn once and held in system memory, then drawn on demand by the OnPaint method at the current scale. The subsequent layers are then drawn for the current clipping region:

private bool _isCalculating = false;

/// <inheritdoc/>
protected override async void OnPaint(PaintEventArgs e) {
  if(e==null) throw new ArgumentNullException("e");
  if (DesignMode) { e.Graphics.FillRectangle(Brushes.Gray, ClientRectangle);  return; }

  if (_isCalculating) return;

  if (BufferCache == null) {
    _isCalculating = true;
    BufferCache    = await PaintedCacheBufferAsync("Cache");
    _isCalculating = false;
    this.Refresh();

  } else {

    var mapScale = DataContext.Scales[ScaleIndex];
    var location = AutoScrollPosition + Margin.OffsetSize();

    BufferCache.Render(BufferMap, mapScale/CacheScale, location);

    BufferMap.Render(BufferUnits, mapScale, location, DataContext.Model.PaintUnits);

    BufferUnits.Render(BufferBack, mapScale, location, DataContext.Model.PaintHighlight);

    BufferShading.Render(BufferBack, mapScale, location, DataContext.Model.PaintShading);

    e.Graphics.DrawImageUnscaled(BufferBack, Point.Empty);
  }
}

Though it works and is reasonably performant, I am not completely happy with the above. In particular, comments are welcome on:

• My obvious assumption about the thread-safety of isCalculating; and
• The overall structure of the OnPaint method.

For completeness, here are the Extension Methods on Image and Bitmap that allow the code above to be fairly neat:

/// <summary>Renders the supplied <see cref="Image"/> <paramref name="source"/> to the specified
/// <see cref="Image"/> <paramref name="target"/>, scaled and translated.</summary>
/// <param name="source">Source <see cref="Image"/> to be rendered.</param>
/// <param name="target">Target <see cref="Graphics"/> to be rendered to.</param>
/// <param name="scale">Scale at which the source should be drawn</param>
/// <param name="point"><see cref="Point"/> at which to render the <paramref name="source"/>.</param>
public static void Render(this Image source, Image target, float scale, Point point) {
  Traces.Paint.Trace("Render source to {0}:",target.Tag);

  using (var gBuffer = Graphics.FromImage(target)) source.Render(gBuffer, scale, point);
}

/// <summary>Renders the supplied <see cref="Image"/> <paramref name="source"/> to the specified
/// <see cref="Graphics"/> <paramref name="target"/>, scaled and translated.</summary>
/// <param name="source">Source <see cref="Image"/> to be rendered.</param>
/// <param name="target">Target <see cref="Graphics"/> to be rendered to.</param>
/// <param name="scale">Scale at which the source should be drawn</param>
/// <param name="point"><see cref="Point"/> at which to render the <paramref name="source"/>.</param>
public static void Render(this Image source, Graphics target, float scale, Point point) {
  target.Clear(Color.Black);

  target.PageUnit = GraphicsUnit.Pixel;
  target.TranslateTransform(point.X, point.Y);
  target.ScaleTransform(scale, scale);

  target.InterpolationMode = InterpolationMode.HighQualityBicubic;
  target.DrawImage(source, Point.Empty);
}

/// <summary>Renders, scaled and translated, first the supplied <see cref="Image"/> <paramref name="source"/> to the specified
/// <see cref="Graphics"/> <paramref name="target"/> and then the specified <paramref name="action"/>.</summary>
/// <param name="source">Source <see cref="Image"/> to be rendered.</param>
/// <param name="target">Target <see cref="Graphics"/> to be rendered to.</param>
/// <param name="scale">Scale at which the source should be drawn</param>
/// <param name="point"><see cref="Point"/> at which to render the <paramref name="source"/>.</param>
/// <param name="action">The drawing action to be overlain to target.</param>
public static void Render(this Image source, Image target, float scale, Point point, Action<Graphics> action) {
  Traces.Paint.Trace("Render cache to {0}:",target.Tag);

  using (var gTarget = Graphics.FromImage(target)) {
    gTarget.DrawImageUnscaled(source, Point.Empty);;

    gTarget.PageUnit = GraphicsUnit.Pixel;
    gTarget.TranslateTransform(point.X, point.Y);
    gTarget.ScaleTransform(scale,scale);

    action(gTarget);
  }
}

Answer 1

Well, "thread-safety" is a fairly vague term. Read/Write access to basic data types like bool for example is guaranteed to be atomic so in that sense access to _isCalculating is thread-safe. However your method is not thread-safe in the general sense as in that if two threads call it at the same time they could very well both be passing the guard check and both be performing the expensive operation.

The call order would be:

• Both threads check _isCalculating at the same time which is false and continue
• Both threads check that BufferCache is null and continue
• Both thread perform the expensive operation

In the general sense protecting entry into a function with a guard flag would have to utilize Interlocked.CompareExchange:

int _IsBusy = 0;

void ExpensiveFunction()
{
    if (Interlocked.CompareExchange(ref _IsBusy, 1, 0) == 1)
    {
        // flag is already set -> don't continue
        return;
    }
    try
    {
        // do this in a try-finally block to avoid being caught out by
        // unexpected exceptions keeping the flag set forever
        PerformExpensiveWork();
    }
    finally
    {
        Interlocked.Exchange(ref _IsBusy, 0);
    }
}

That being said, given that your method is called OnPaint I assume it's only ever going to be called from a UI thread and of that there is usually only one. So your current implementation will probably work but that's largely due to the fact that calls to OnPaint are going to be made from a single thread rather than your implementation being thread-safe.

One issue, as mentioned in the above example code, is that should the expensive operation (PaintedCacheBufferAsync in your case) throw in any way then you have locked yourself out because the _isCalculating flag will never be reset.

Answer 2

ChrisWue raised two valid points, but the implementation suggested in his answer is faulty - simply swirling the mouse over the expensive control as it painted the cache caused an InvalidOperationException in Image.cs. The following variation on his implementation works and repeatedly survives the swirl test: private int _isBusy = 0;

/// <inheritdoc/>
protected override async void OnPaint(PaintEventArgs e) {
  if(e==null) throw new ArgumentNullException("e");
  if (DesignMode) { e.Graphics.FillRectangle(Brushes.Gray, ClientRectangle);  return; }

#if FIRST_WAY  // both of these work fine
  if(Interlocked.CompareExchange(ref _isBusy, 1, 1) == 1) return;
#else
  if(_isBusy == 1) return;
#endif

  if (BufferCache == null) {
    try {
      Interlocked.Exchange(ref _isBusy, 1);
      BufferCache  = await PaintedCacheBufferAsync("Cache");
    } finally { Interlocked.Exchange(ref _isBusy,0); }
    this.Refresh();
  } else {
    var mapScale = DataContext.Scales[ScaleIndex];
    var location = AutoScrollPosition + Margin.OffsetSize();

    BufferCache.Render(BufferMap, mapScale/CacheScale, location);

    BufferMap.Render(BufferUnits, mapScale, location, DataContext.Model.PaintUnits);

    BufferUnits.Render(BufferBack, mapScale, location, DataContext.Model.PaintHighlight);

    BufferShading.Render(BufferBack, mapScale, location, DataContext.Model.PaintShading);

    e.Graphics.DrawImageUnscaled(BufferBack, Point.Empty);
  }
}

Update - this implementation is better:

private int _isBusy = 0;

/// <inheritdoc/>
protected override async void OnPaint(PaintEventArgs e) {
  if(e==null) throw new ArgumentNullException("e");
  if (DesignMode) { e.Graphics.FillRectangle(Brushes.Gray, ClientRectangle);  return; }

  if (BufferCache != null) {
    var mapScale = DataContext.Scales[ScaleIndex];
    var location = AutoScrollPosition + Margin.OffsetSize();

    BufferCache.Render(BufferMap, mapScale/CacheScale, location);

    BufferMap.Render(BufferUnits, mapScale, location, DataContext.Model.PaintUnits);

    BufferUnits.Render(BufferBack, mapScale, location, DataContext.Model.PaintHighlight);

    BufferShading.Render(BufferBack, mapScale, location, DataContext.Model.PaintShading);

    e.Graphics.DrawImageUnscaled(BufferBack, Point.Empty);
  } else if (Interlocked.CompareExchange(ref _isBusy, 1, 0) == 0) {
    try     { BufferCache  = await PaintedCacheBufferAsync("Cache"); }
    finally { Interlocked.Exchange(ref _isBusy, 0); }
    this.Refresh();
  }
}

This passes the swirl test. There is still a potential race condition between this code fragment and the one location where BufferCache is set (to null, when scale changes), but currently that only occurs in the control constructor. Addressing it fully would require full access locking I believe.