Skip to content

fix: improve null safety in ReadySystem, MatchManager, and MatchUtility#135

Merged
lukepolo merged 2 commits intomainfrom
audit/399-null-safety
Mar 31, 2026
Merged

fix: improve null safety in ReadySystem, MatchManager, and MatchUtility#135
lukepolo merged 2 commits intomainfrom
audit/399-null-safety

Conversation

@Flegma
Copy link
Copy Markdown
Contributor

@Flegma Flegma commented Mar 30, 2026

Summary

  • ReadySystem.SendReadyMessage: Use TryGetValue instead of direct dictionary indexer to prevent KeyNotFoundException when a player reconnects and isn't yet in _readyPlayers
  • MatchManager.GetCurrentMap: Capture _matchData in a local variable before passing to lambda, preventing a race condition if the field becomes null between the null check and lambda execution
  • MatchUtility.GetPlayerLineupTag: Replace tag == null || tag == "" with idiomatic string.IsNullOrEmpty(tag)

Test plan

  • Player reconnecting during ready phase doesn't crash the server
  • Current map lookup works correctly during active matches
  • Player lineup tags display correctly (including empty/null tags)

Closes 5stackgg/5stack-panel#399

- ReadySystem.SendReadyMessage: use TryGetValue instead of direct
  dictionary access to prevent KeyNotFoundException on reconnect
- MatchManager.GetCurrentMap: capture _matchData in local variable
  before lambda to prevent race condition if field becomes null
- MatchUtility.GetPlayerLineupTag: use string.IsNullOrEmpty() instead
  of manual null/empty check

Closes 5stackgg/5stack-panel#399
Comment thread src/FiveStack.Services/ReadySystem.cs Outdated
bool isReady = _readyPlayers[player.UserId.Value];
if (!_readyPlayers.TryGetValue(player.UserId.Value, out bool isReady))
{
return;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we return false?

Per review: if player isn't in the dictionary, TryGetValue defaults
isReady to false, so they'll see the "not ready" message.
@Flegma
Copy link
Copy Markdown
Contributor Author

Flegma commented Mar 31, 2026

Good call — now if the player isn't in the dictionary, TryGetValue defaults isReady to false and they'll see the "not ready" message instead of the method silently returning.

@lukepolo lukepolo merged commit e1f190a into main Mar 31, 2026
1 check passed
@lukepolo lukepolo deleted the audit/399-null-safety branch March 31, 2026 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Game Server] Null reference risks in MatchManager, ReadySystem, MatchUtility

2 participants