Skip to content

Commit

Permalink
fix rare race condition with asteroid cache
Browse files Browse the repository at this point in the history
On a very rare occasion, a race would result in a collection was
modified exception would be thrown when traversing the collection
of asteroids in the CacheManager.  This adds a lock to block when
accessed from multiple threads.
  • Loading branch information
aesalazar committed Apr 22, 2024
1 parent e6ad573 commit d5ea9c7
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 25 deletions.
61 changes: 41 additions & 20 deletions Asteroids.Standard/Managers/CacheManager.cs
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
using System.Collections.Generic;
using System.Collections.ObjectModel;
using Asteroids.Standard.Components;
using Asteroids.Standard.Screen;
using System.Collections.Generic;
using System.Drawing;
using System.Linq;
using Asteroids.Standard.Components;
using Asteroids.Standard.Screen;

namespace Asteroids.Standard.Managers
{
Expand All @@ -23,15 +22,16 @@ public CacheManager(ScoreManager score, Ship? ship, AsteroidBelt belt, IList<Bul
Score = score;
Ship = ship;
Belt = belt;
_bullets = bullets;
_bullets = bullets.ToList();

_bulletLock = new object();
_explosionLock = new object();
_asteroidsLock = new object();

_explosions = new List<Explosion>();
_bulletsInFlight = new List<CachedObject<Bullet>>();
_bulletsAvailable = new List<CachedObject<Bullet>>();
Asteroids = new List<CachedObject<Asteroid>>();
_asteroids = new List<CachedObject<Asteroid>>();

Repopulate();
}
Expand All @@ -43,11 +43,13 @@ public CacheManager(ScoreManager score, Ship? ship, AsteroidBelt belt, IList<Bul
//Read-only
private readonly object _bulletLock;
private readonly object _explosionLock;
private readonly object _asteroidsLock;

private readonly IList<Explosion> _explosions;
private readonly IList<Bullet> _bullets;
private readonly IList<CachedObject<Bullet>> _bulletsInFlight;
private readonly IList<CachedObject<Bullet>> _bulletsAvailable;
private readonly List<Explosion> _explosions;
private readonly List<Bullet> _bullets;
private readonly List<CachedObject<Bullet>> _bulletsInFlight;
private readonly List<CachedObject<Bullet>> _bulletsAvailable;
private readonly List<CachedObject<Asteroid>> _asteroids;

public ScoreManager Score { get; }

Expand All @@ -66,11 +68,6 @@ public CacheManager(ScoreManager score, Ship? ship, AsteroidBelt belt, IList<Bul
/// </summary>
public AsteroidBelt Belt { get; private set; }

/// <summary>
/// Collection of cached <see cref="Asteroid"/>s with parameters cached for optimization.
/// </summary>
public IList<CachedObject<Asteroid>> Asteroids { get; private set; }

/// <summary>
/// Collection of points associated with the <see cref="Saucer"/>, if present.
/// </summary>
Expand Down Expand Up @@ -155,10 +152,28 @@ public void UpdateBelt(AsteroidBelt belt)
{
Belt = belt;

Asteroids = Belt
var list = Belt
.GetAsteroids()
.Select(a => new CachedObject<Asteroid>(a))
.ToList();

lock (_asteroidsLock)
{
_asteroids.Clear();
_asteroids.AddRange(list);
}
}

/// <summary>
/// Returns the current collection of cached Asteroids in a thread-safe manor.
/// </summary>
/// <returns>Disconnected list of cached Asteroids.</returns>
public IList<CachedObject<Asteroid>> GetAsteroids()
{
lock (_asteroidsLock)
{
return _asteroids.ToList();
}
}

/// <summary>
Expand All @@ -167,8 +182,11 @@ public void UpdateBelt(AsteroidBelt belt)
/// </summary>
public void AddAsteroid(Asteroid asteroid)
{
Asteroids.Add(new CachedObject<Asteroid>(asteroid));
Belt.SetAsteroids(Asteroids.Select(c => c.ScreenObject).ToList());
lock (_asteroidsLock)
{
_asteroids.Add(new CachedObject<Asteroid>(asteroid));
Belt.SetAsteroids(_asteroids.Select(c => c.ScreenObject).ToList());
}
}

/// <summary>
Expand All @@ -177,8 +195,11 @@ public void AddAsteroid(Asteroid asteroid)
/// </summary>
public void RemoveAsteroid(int index)
{
Asteroids.RemoveAt(index);
Belt.SetAsteroids(Asteroids.Select(c => c.ScreenObject).ToList());
lock (_asteroidsLock)
{
_asteroids.RemoveAt(index);
Belt.SetAsteroids(_asteroids.Select(c => c.ScreenObject).ToList());
}
}

/// <summary>
Expand Down
6 changes: 3 additions & 3 deletions Asteroids.Standard/Managers/CollisionManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public CollisionManager(CacheManager cache)
public bool AsteroidBeltCollision(IList<Point> pointsToCheck)
{
//Get a copy for collection adds/removes
var asteroids = _cache.Asteroids.ToList();
var asteroids = _cache.GetAsteroids();
var score = 0;

//Go through each but break on first hit
Expand Down Expand Up @@ -131,7 +131,7 @@ public bool IsCenterSafe()
{
bool safe = true;

foreach (var asteroid in _cache.Asteroids)
foreach (var asteroid in _cache.GetAsteroids())
{
var separation = asteroid
.Location
Expand Down Expand Up @@ -222,7 +222,7 @@ public void MoveBullets()
public void MoveAsteroids()
{
//Move does not use locks
foreach (var asteroid in _cache.Asteroids)
foreach (var asteroid in _cache.GetAsteroids())
asteroid.ScreenObject.Move();
}

Expand Down
2 changes: 1 addition & 1 deletion Asteroids.Standard/Managers/DrawingManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ private void DrawBullets()
private void DrawBelt()
{
var asteroids = _cache
.Asteroids
.GetAsteroids()
.Where(a => a.ScreenObject.Size != Asteroid.AsteroidSize.Dne);

foreach (var asteroid in asteroids)
Expand Down
2 changes: 1 addition & 1 deletion Asteroids.Standard/Screen/TitleScreen.cs
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ public void DrawScreen()
// Draw the asteroid belt
_cache.Repopulate();

foreach (var asteroid in _cache.Asteroids)
foreach (var asteroid in _cache.GetAsteroids())
{
asteroid.ScreenObject.Move();
_canvas.LoadPolygon(asteroid.PolygonPoints, DrawColor.White);
Expand Down

0 comments on commit d5ea9c7

Please sign in to comment.