Thread-safety concern in PlayerManager.MarkPlayerDirty method

[coderabbitai]

Issue Description

A thread-safety concern was identified in the PlayerManager.MarkPlayerDirty method. The method currently uses a read lock to look up the player in the dictionary, but then modifies player.ChangesDetected and player.CharacterChangesDetected without write protection.

While the read lock guards dictionary access, it doesn't synchronize writes to the player object itself. This is problematic if MarkPlayerDirty is intended to be callable from non-world-thread contexts (e.g., SaveScheduler callbacks).

For comparison, the callback at line 229 uses a write lock when setting ChangesDetected in a similar situation.

Recommendation

Either:

1. Use a write lock instead of a read lock for the entire lookup-and-modify sequence, or
2. Document that this method may only be called from the world thread and cannot be invoked from external threads to avoid races.

References

• Pull Request: Improve save reliability and logout safety #314
• Review Comment: Improve save reliability and logout safety #314 (comment)
• Requested by: @rkroska

File Location

Source/ACE.Server/Managers/PlayerManager.cs around lines 482-503

[CrimsonMage]

This can be closed as of the merge of Pull request 322.