Refactor FieldManager.Factory.cs to add thread-safe access to field creation - #275
Conversation
WalkthroughThe changes introduce a Changes
Assessment against linked issues
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
Maple2.Server.Game/Manager/Field/FieldManager.Factory.cs (1)
36-36: SimplifySemaphoreSliminitializationYou can simplify the initialization of
SemaphoreSlimby using its constructor that only requires the initial count when it's equal to the maximum count.Replace the current initialization with:
- semaphore = new SemaphoreSlim(initialCount: 1, maxCount: 1); + semaphore = new SemaphoreSlim(1);This makes the code more concise without changing the behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- Maple2.Server.Game/Manager/Field/FieldManager.Factory.cs (2 hunks)
🔇 Additional comments (1)
Maple2.Server.Game/Manager/Field/FieldManager.Factory.cs (1)
48-66: Ensure exception safety when refactoringIf you choose to refactor the
Getmethods to eliminate the semaphore, make sure that theCreatemethod is thread-safe and handles concurrent calls appropriately. IfCreatecan be called simultaneously by multiple threads for the samemapId, ensure that it does not result in duplicate fields or unintended side effects.Consider verifying the thread safety of the
Createmethod. Run the following script to search for any usages of shared resources withinCreatethat may not be thread-safe:Also applies to: 74-88
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- Maple2.Server.Game/Manager/Field/FieldManager.Factory.cs (6 hunks)
🔇 Additional comments (4)
Maple2.Server.Game/Manager/Field/FieldManager.Factory.cs (4)
31-32: LGTM: Thread-safe lock management implementedThe addition of
mapLocksas aConcurrentDictionary<int, SemaphoreSlim>is a good approach for managing locks on a per-map basis. This implementation aligns well with the PR objective of adding thread-safe access to field creation.Also applies to: 35-35
184-187: LGTM: Proper resource management and lock retrieval implementedThe changes to the
Disposemethod ensure that all semaphore locks are properly disposed of, preventing resource leaks. The newGetMapLockmethod provides a thread-safe way to retrieve or create semaphore locks for each map ID. These implementations contribute to the overall thread safety and resource management of the class.Also applies to: 193-196
Line range hint
1-196: Summary: Thread-safe field creation implemented with some suggestions for improvementThe changes in this PR successfully address the issue of multiple players creating separate instances of the same non-instanced map by implementing thread-safe access to field creation. The use of
ConcurrentDictionaryfor managing locks andSemaphoreSlimfor synchronization is appropriate and aligns well with the PR objectives.However, there are a few areas where the implementation could be improved:
- Ensure proper lock release in the
Getmethods by usingtry-finallyblocks.- Consider refactoring the lock management for better encapsulation and maintainability.
These improvements will further enhance the thread safety and reliability of the field creation process. Overall, the PR makes significant progress towards resolving the issue described in #268.
71-73:⚠️ Potential issueApply the same lock release pattern as suggested for the previous
GetmethodThis method has the same lock management issue as the previous
Getmethod. Please apply the sametry-finallyblock pattern here to ensure proper lock release.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
Maple2.Server.Game/Manager/Field/FieldManager.Factory.cs (1)
182-210: LGTM with suggestion: Consider refactoring to reduce duplicationThe new
GetInternalmethods correctly encapsulate the logic previously contained in theGetmethods. This separation of concerns improves readability and maintainability of the code.However, there's noticeable similarity between the two
GetInternalmethods. Consider refactoring to reduce this duplication. For example, you could create a single method that handles both cases:private FieldManager? GetInternal(int mapId, long ownerId = 0, int instanceId = 0) { if (!fields.TryGetValue(mapId, out OwnerFields? ownerFields)) { return Create(mapId, ownerId: ownerId, instanceId: instanceId); } if (!ownerFields.TryGetValue(ownerId, out InstancedFields? instancedFields)) { return Create(mapId, ownerId: ownerId, instanceId: instanceId); } return instancedFields.TryGetValue(instanceId, out FieldManager? field) ? field : Create(mapId, ownerId: ownerId, instanceId: instanceId); }This refactoring would eliminate the need for two separate methods while maintaining the same functionality.
Maple2.Server.Game/Manager/Field/FieldManager.State.cs (3)
454-454: LGTM! Consider adding error logging.The addition of
bonusMap?.Init();is a good practice to ensure the bonus map is properly initialized before use. This change aligns with the PR objectives to enhance thread safety in field creation.Consider adding error logging if the bonus map creation fails. This can be done by modifying the code as follows:
-bonusMap?.Init(); +if (bonusMap != null) +{ + bonusMap.Init(); +} +else +{ + Log.Error("Failed to create bonus map {MapId}", bonusMapMetadata.Id); +}This change will provide better visibility into potential issues during bonus map creation.
Line range hint
451-459: Consider improving error handling for bonus map creationThe current implementation handles the case where bonus map creation fails, but it continues to create a portal even if the bonus map is null. This could lead to a portal that doesn't lead anywhere.
Consider refactoring the code to return early if the bonus map creation fails:
MapMetadata bonusMapMetadata = bonusMaps[Random.Shared.Next(bonusMaps.Count)]; FieldManager? bonusMap = FieldFactory.Create(bonusMapMetadata.Id); bonusMap?.Init(); Console.WriteLine($"Creating bonus map {bonusMapMetadata.Id} at {spawn.Position} in {delay} ms."); if (bonusMap == null) { + Log.Error("Failed to create bonus map {MapId}", bonusMapMetadata.Id); + Scheduler.Schedule(() => SetBonusMapPortal(bonusMaps, spawn), delay); return; } bonusMap.SetRoomTimer(RoomTimerType.Clock, 90000);This change ensures that we don't create a portal for a non-existent bonus map, while still scheduling the next attempt to create a bonus map.
Line range hint
1-1000: Well-structured class with good thread-safety practicesThe
FieldManagerclass is well-structured and implements good practices for thread-safety, such as using concurrent collections and nullable reference types. This aligns well with the PR objectives of enhancing thread safety in field creation.For consistency, consider using the null-conditional operator
?.in other similar situations throughout the class. For example, in theRemoveNpcmethod:public bool RemoveNpc(int objectId, int removeDelay = 0) { if (!Mobs.TryRemove(objectId, out FieldNpc? npc) && !Npcs.TryRemove(objectId, out npc)) { return false; } Scheduler.Schedule(() => { Broadcast(FieldPacket.RemoveNpc(objectId)); Broadcast(ProxyObjectPacket.RemoveNpc(objectId)); - npc.Dispose(); + npc?.Dispose(); }, removeDelay); return true; }This change would make the code more consistent with the null-safety practices used elsewhere in the class.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- Maple2.Server.Game/Manager/Field/FieldManager.Factory.cs (3 hunks)
- Maple2.Server.Game/Manager/Field/FieldManager.State.cs (1 hunks)
🔇 Additional comments (7)
Maple2.Server.Game/Manager/Field/FieldManager.Factory.cs (7)
31-32: LGTM: Thread-safe lock management implementedThe addition of
mapLocksas aConcurrentDictionary<int, SemaphoreSlim>is a good approach for managing locks on a per-map basis. This implementation allows for efficient concurrent access and helps address the issue of multiple players entering the same non-instanced map simultaneously.
35-35: LGTM: Proper initialization of mapLocksThe initialization of
mapLocksin the constructor is correct and necessary. This ensures that the lock management system is ready for use as soon as theFactoryinstance is created.
47-55: LGTM: Proper implementation of thread-safe field retrievalThe changes to the
Getmethod effectively implement thread-safe access to field creation:
- The lock is correctly acquired using
GetMapLock(mapId).- The
try-finallyblock ensures that the lock is always released, even if an exception occurs.- Moving the logic to a separate
GetInternalmethod improves code organization and readability.These changes directly address the issue of multiple players creating separate instances when entering the same non-instanced map simultaneously.
64-72: LGTM: Consistent implementation of thread-safety in overloaded Get methodThe changes in this overloaded
Getmethod mirror those in the previous method, ensuring consistent thread-safe behavior:
- The lock is acquired using
GetMapLock(mapId).- The
try-finallyblock is used to guarantee lock release.- The logic is delegated to a separate
GetInternalmethod.This consistency across method overloads is crucial for maintaining uniform behavior and thread-safety throughout the class.
169-172: LGTM: Proper disposal of SemaphoreSlim instancesThe addition of the loop to dispose of all
SemaphoreSliminstances inmapLocksis a crucial step in preventing resource leaks. This ensures that all system resources associated with the semaphores are properly released when theFactoryis disposed.
178-180: LGTM: Efficient and thread-safe lock managementThe
GetMapLockmethod is well-implemented:
- It uses
GetOrAddto ensure thread-safe creation of new locks when needed.- It efficiently manages the creation and retrieval of locks for each map ID.
- The method encapsulates the lock management logic, promoting code reusability and maintainability.
This implementation supports the overall goal of ensuring thread-safe access to field creation.
Line range hint
1-238: Summary: Effective implementation of thread-safe field creationThe changes in this PR successfully address the issue of multiple players creating separate instances when entering the same non-instanced map simultaneously. Key improvements include:
- Introduction of a
ConcurrentDictionary<int, SemaphoreSlim>for efficient, thread-safe lock management.- Implementation of locking mechanisms in the
Getmethods to ensure thread-safe access to field creation.- Proper disposal of
SemaphoreSliminstances to prevent resource leaks.- Encapsulation of lock management and field retrieval logic in separate methods for improved code organization.
These changes effectively meet the objectives outlined in the PR and linked issue #268. The code is well-structured and maintainable, with only a minor suggestion for refactoring to reduce duplication in the
GetInternalmethods.Overall, this PR represents a significant improvement in the thread-safety and reliability of the field creation process.
Summary by CodeRabbit
These changes improve the reliability of the application, ensuring smoother performance during high-traffic scenarios.