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

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;

/// 
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:

```
/// Renders the supplied to the specified
/// , scaled and translated.
/// Source to be rendered.
/// Target to be rendered to.
/// Scale at which the source should be drawn
/// at which to render the .
public static voi

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.

StackExchange Code Review Q#78076, answer score: 3