Skip to content

modify: change map cooldown behaviour #221

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fltuna merged 3 commits into from
Sep 24, 2025
Merged

modify: change map cooldown behaviour #221

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5671452
modify: change map cooldown behaviour
fltuna Sep 24, 2025
1786a1a
modify: map cooldown now applied asynchronously (completely)
fltuna Sep 24, 2025
6209326
modify: refactor
fltuna Sep 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 31 additions & 17 deletions MapChooserSharp/Modules/MapCycle/McsMapCycleController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,11 +264,23 @@ private void OnClientPutInServer(int slot)
private void OnMapStart(string mapName)
{
ExtendCount = 0;

DecrementAllMapCooldown(CurrentMap);

CurrentMap = NextMap;
NextMap = null;

Task.Run(async () =>
{
try
{
await DecrementAllMapCooldown();
await ApplyCooldownToCurrentMap(CurrentMap);
}
catch (Exception e)
{
Logger.LogError(e, "Exception occured while applying map/group cooldown.");
}
});
Comment on lines 268 to +282
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Sep 24, 2025

Choose a reason for hiding this comment

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

critical

There's a logic error in how the map cooldown is applied. CurrentMap is updated to the new map on line 268, before ApplyCooldownToCurrentMap is called. This results in the cooldown being applied to the map that is just starting, not the one that just finished. You should capture the previous map's configuration before updating CurrentMap and use that to apply the cooldown.

        var previousMap = CurrentMap;
        CurrentMap = NextMap;
        NextMap = null;

        Task.Run(async () =>
        {
            try
            {
                await DecrementAllMapCooldown();
                await ApplyCooldownToCurrentMap(previousMap);
            }
            catch (Exception e)
            {
                Logger.LogError(e, "Exception occured while applying map/group cooldown.");
            }
        });

Comment on lines +271 to +282
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Sep 24, 2025

Choose a reason for hiding this comment

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

medium

Using Task.Run for fire-and-forget operations can be risky. If the application shuts down unexpectedly, this background task could be terminated abruptly. This might leave the database in an inconsistent state if the operations are not atomic. While the previous implementation had a similar fire-and-forget behavior, it's worth considering a more robust approach for background tasks that ensures completion or graceful cancellation on shutdown, especially for critical operations like database updates.

Copy link
Copy Markdown
Owner Author

@fltuna fltuna Sep 24, 2025

Choose a reason for hiding this comment

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

@gemini-code-assist So how do I needs to be? iirc I'm implemented a DB operation as a atomic, so I didn't care about fire-and-forget pattern.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Sep 24, 2025

Choose a reason for hiding this comment

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

You're right that individual database operations can be atomic, ensuring data integrity at the database level for each specific write. However, the concern with a "fire-and-forget" Task.Run here is about the sequence of operations (DecrementAllMapCooldown and ApplyCooldownToCurrentMap). If the application shuts down unexpectedly between these two await calls, even if each is atomic, the overall application state regarding map cooldowns could become inconsistent.

The most robust way to ensure these critical operations complete is to await them directly. This would require changing the OnMapStart method's signature from private void to private async Task.

If OnMapStart can be made async Task, I suggest the following change to directly await the operations, which ensures that the method will not complete until these background tasks are finished. This eliminates the "fire-and-forget" aspect and provides better control over the task's lifecycle.

If, due to external constraints (e.g., OnMapStart being an event handler that must have a void return type), you cannot change its signature to async Task, then Task.Run with the try-catch block (as you currently have) is a common workaround to prevent blocking the main thread. However, it's important to be aware that in such async void scenarios, unhandled exceptions can still crash the application, and the task's completion cannot be directly observed or awaited by the caller, maintaining the risk of abrupt termination on application shutdown.

Suggested change
Task.Run(async () =>
{
try
{
await DecrementAllMapCooldown();
await ApplyCooldownToCurrentMap(CurrentMap);
}
catch (Exception e)
{
Logger.LogError(e, "Exception occured while applying map/group cooldown.");
}
});
try
{
await DecrementAllMapCooldown();
await ApplyCooldownToCurrentMap(CurrentMap);
}
catch (Exception e)
{
Logger.LogError(e, "Exception occured while applying map/group cooldown.");
}

Copy link
Copy Markdown
Owner Author

@fltuna fltuna Sep 24, 2025

Choose a reason for hiding this comment

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

@gemini-code-assist no this is in main game loop so I use Task.Run() as an async

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Sep 24, 2025

Choose a reason for hiding this comment

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

Thank you for clarifying that OnMapStart is part of the main game loop. Given this constraint, where the method likely needs to remain private void, using Task.Run is indeed the correct approach to offload the asynchronous database operations (DecrementAllMapCooldown and ApplyCooldownToCurrentMap) without blocking the main thread.

Your current implementation, which wraps these operations in a Task.Run with a try-catch block, is a standard pattern for handling asynchronous work in async void contexts. This prevents the main thread from being blocked and handles potential exceptions within the background task.

Just to reiterate the point from my previous comment, while this approach is necessary due to the void return type, it still carries the inherent characteristics of "fire-and-forget" tasks:

  • Unhandled Exceptions: Although you have a try-catch block, if an exception were to escape this block within the Task.Run delegate, it could still crash the application, as exceptions in async void methods are re-thrown on the SynchronizationContext (or the ThreadPool if none is present).
  • Application Shutdown: If the application shuts down while these tasks are still running, they might be terminated abruptly, potentially leaving the database in an inconsistent state if the operations are not fully atomic or transactional at a higher level than individual writes. You mentioned your DB operations are atomic, which helps mitigate this, but the sequence of operations still has a window of vulnerability.

For this specific scenario, your current implementation is a pragmatic solution given the void return type requirement. It correctly moves the asynchronous work off the main thread and includes basic error handling.



Server.NextFrame(ExecuteMapLimitConfig);

Expand Down Expand Up @@ -303,15 +315,15 @@ private void ObtainCurrentMap(string mapName)
ExtendLimit = CurrentMap?.MaxExtends ?? DefaultMapExtends;
}

private void DecrementAllMapCooldown(IMapConfig? previousMap)
private async Task DecrementAllMapCooldown()
{
// To prevent unxpected cooldown reduction
if (!IsFirstMapEnded || !IsSecondMapIsPassed)
return;

// Decrement all cooldowns
_mcsDatabaseProvider.MapInfoRepository.DecrementAllCooldownsAsync().ConfigureAwait(false);
_mcsDatabaseProvider.GroupInfoRepository.DecrementAllCooldownsAsync().ConfigureAwait(false);
await _mcsDatabaseProvider.MapInfoRepository.DecrementAllCooldownsAsync();
await _mcsDatabaseProvider.GroupInfoRepository.DecrementAllCooldownsAsync();
foreach (var (key, value) in _mcsInternalMapConfigProviderApi.GetMapConfigs())
{
foreach (IMapGroupSettings setting in value.GroupSettings)
Expand All @@ -323,22 +335,24 @@ private void DecrementAllMapCooldown(IMapConfig? previousMap)
if (value.MapCooldown.CurrentCooldown > 0)
value.MapCooldown.CurrentCooldown--;
}

// Set previous map cooldown if defined in config
if (previousMap != null)
{
_mcsDatabaseProvider.MapInfoRepository.UpsertMapCooldownAsync(previousMap.MapName, previousMap.MapCooldown.MapConfigCooldown).ConfigureAwait(false);
previousMap.MapCooldown.CurrentCooldown = previousMap.MapCooldown.MapConfigCooldown;
}

private async Task ApplyCooldownToCurrentMap(IMapConfig? currentMap)
{
if (currentMap == null)
return;

foreach (IMapGroupSettings setting in previousMap.GroupSettings)
{
_mcsDatabaseProvider.GroupInfoRepository.UpsertGroupCooldownAsync(setting.GroupName, setting.GroupCooldown.MapConfigCooldown).ConfigureAwait(false);
setting.GroupCooldown.CurrentCooldown = setting.GroupCooldown.MapConfigCooldown;
}
await _mcsDatabaseProvider.MapInfoRepository.UpsertMapCooldownAsync(currentMap.MapName, currentMap.MapCooldown.MapConfigCooldown);
currentMap.MapCooldown.CurrentCooldown = currentMap.MapCooldown.MapConfigCooldown;

foreach (IMapGroupSettings setting in currentMap.GroupSettings)
{
await _mcsDatabaseProvider.GroupInfoRepository.UpsertGroupCooldownAsync(setting.GroupName, setting.GroupCooldown.MapConfigCooldown);
setting.GroupCooldown.CurrentCooldown = setting.GroupCooldown.MapConfigCooldown;
}
}



private HookResult OnRoundEnd(EventRoundEnd @event, GameEventInfo info)
{
if (!_isMapStarted)
Expand Down
Loading