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 2 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 void() =>
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 async void for lambdas passed to Task.Run is generally discouraged. While it works here because of the try-catch block, it's a less robust pattern. async void methods have different error-handling semantics and are harder to test and compose.

It's better to use an async Task lambda (async () => { ... }). Task.Run has an overload that accepts a Func<Task>, and this is the preferred way to run async operations on a background thread. The Task returned by Task.Run will then represent the entire lifetime of the async operation inside the lambda, which is generally what's expected and a more robust pattern.

        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 +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