Enhance configuration options with auto-blacklist, delay, and retry settings #126
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @peter9811, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request introduces new configuration options to the ASFFreeGames plugin, enhancing its behavior with automatic blacklisting of forbidden packages, adjustable delays between requests, and retry mechanisms for transient errors. It also adds commands to manage the blacklist via chat.
Highlights
- Blacklist Management: Adds commands to show, clear, and remove entries from the blacklist via chat interface.
- Automatic Blacklisting: Introduces an option to automatically blacklist packages that return 'Forbidden' errors.
- Request Throttling: Adds configuration options for setting delays between requests and retrying failed requests due to transient errors.
- Error Handling: Improves error handling by retrying transient errors and logging more detailed status messages.
Changelog
Click here to see the changelog
- ASFFreeGames/Commands/FreeGamesCommand.cs
- Adds 'SHOWBLACKLIST' command to display the current blacklist.
- Implements 'CLEARBLACKLIST' command to clear all entries from the blacklist.
- Introduces 'REMOVEBLACKLIST' command to remove a specific package from the blacklist.
- Adds a cache to skip packages that have already failed in the current collection run.
- Implements retry logic for transient errors with configurable delay and max attempts.
- Adds automatic blacklisting of packages that return 'Forbidden' errors.
- Adds a delay between requests to avoid rate limits.
- Logs more detailed status messages for each game collection attempt.
- ASFFreeGames/Configurations/ASFFreeGamesOptions.cs
- Adds 'AutoBlacklistForbiddenPackages' option to automatically blacklist forbidden packages.
- Adds 'DelayBetweenRequests' option to set a delay between license requests.
- Adds 'MaxRetryAttempts' option to configure the number of retry attempts for transient errors.
- Adds 'RetryDelayMilliseconds' option to set the delay between retries.
- Adds methods to add, remove, and clear items from the blacklist.
- README.md
- Adds documentation for the new blacklist management commands.
- Adds documentation for the new configuration options (autoBlacklistForbiddenPackages, delayBetweenRequests, maxRetryAttempts, retryDelayMilliseconds).
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Trivia time!
What is the standard delay that Thread.Sleep() uses if a negative timeout is provided?
Click here for the answer
If the value of the timeout argument is other than Timeout.Infinite, and less than or equal to zero, the thread is not suspended. If the value of timeout is Timeout.Infinite, the thread is suspended indefinitely.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces several new configuration options to enhance the plugin's behavior, including automatic blacklisting, adjustable delays, and retry mechanisms. The changes seem well-structured and address potential issues like rate limiting and transient errors. However, there are a few areas that could benefit from further refinement and clarification.
Summary of Findings
- Inconsistent Blacklist Handling: The blacklist functionality is implemented in both
FreeGamesCommand.csandASFFreeGamesOptions.cs. Consolidating this logic into a single location could improve maintainability. - Potential NullReferenceException: The code uses
Options.MaxRetryAttempts ?? 1and similar constructs. While this provides a default value, it might be better to ensureOptionsitself is not null before accessing its properties to prevent potentialNullReferenceException. - Lack of Centralized Configuration Handling: The configuration options are spread across multiple files. Centralizing the configuration logic could improve readability and maintainability.
Merge Readiness
The pull request introduces valuable enhancements to the plugin. However, before merging, it's recommended to address the identified issues, particularly those related to blacklist handling and potential null reference exceptions. I am unable to approve this pull request, and recommend that others review and approve this code before merging.
| case "REMOVEBLACKLIST": | ||
| if (args.Length >= 4) { | ||
| string identifier = args[3]; | ||
| if (GameIdentifier.TryParse(identifier, out GameIdentifier gid)) { | ||
| bool removed = Options.RemoveFromBlacklist(in gid); | ||
| await SaveOptions(cancellationToken).ConfigureAwait(false); | ||
|
|
||
| if (removed) { | ||
| return FormatBotResponse(bot, $"{ASFFreeGamesPlugin.StaticName} removed {gid} from blacklist"); | ||
| } else { | ||
| return FormatBotResponse(bot, $"{ASFFreeGamesPlugin.StaticName} could not find {gid} in blacklist"); | ||
| } | ||
| } else { | ||
| return FormatBotResponse(bot, $"Invalid game identifier format: {identifier}"); | ||
| } | ||
| } else { | ||
| return FormatBotResponse(bot, "Please provide a game identifier to remove from blacklist"); | ||
| } |
There was a problem hiding this comment.
This block handles the REMOVEBLACKLIST command. Consider adding a check to ensure the identifier is not null or empty before attempting to parse it. This can prevent unexpected behavior or errors.
case "REMOVEBLACKLIST":
if (args.Length >= 4) {
string identifier = args[3];
if (string.IsNullOrEmpty(identifier)) {
return FormatBotResponse(bot, "Please provide a valid game identifier to remove from blacklist");
}
if (GameIdentifier.TryParse(identifier, out GameIdentifier gid)) {| int retryAttempts = 0; | ||
| int maxRetries = Options.MaxRetryAttempts ?? 1; |
There was a problem hiding this comment.
Consider adding a check to ensure that Options is not null before accessing MaxRetryAttempts. While the null-coalescing operator provides a default value, explicitly checking for null can prevent potential NullReferenceException if Options itself is null.
int maxRetries = Options?.MaxRetryAttempts ?? 1;| if (Options.AutoBlacklistForbiddenPackages ?? true) { | ||
| Options.AddToBlacklist(in gid); | ||
|
|
||
| if (VerboseLog || requestSource is ECollectGameRequestSource.RequestedByUser) { | ||
| bot.ArchiLogger.LogGenericInfo($"[FreeGames] Adding {gid} to blacklist due to Forbidden response", nameof(CollectGames)); | ||
| } | ||
|
|
||
| // Save the updated options to persist the blacklist | ||
| _ = Task.Run(async () => { | ||
| try { | ||
| await SaveOptions(cancellationToken).ConfigureAwait(false); | ||
| } catch (Exception ex) { | ||
| if (VerboseLog || requestSource is ECollectGameRequestSource.RequestedByUser) { | ||
| bot.ArchiLogger.LogGenericWarning($"Failed to save options after blacklisting: {ex.Message}", nameof(CollectGames)); | ||
| } | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
The code saves options within a Task.Run. While this prevents blocking the main thread, it's generally better to use Task.ConfigureAwait(false) to avoid potential deadlocks or context-switching issues. Also, consider using ArchiSteamFarm.Core.ASF.ArchiLogger.LogGenericError instead of bot.ArchiLogger.LogGenericWarning to be consistent with the logging style.
_ = Task.Run(async () => {
try {
await SaveOptions(cancellationToken).ConfigureAwait(false);
} catch (Exception ex) {
if (VerboseLog || requestSource is ECollectGameRequestSource.RequestedByUser) {
ArchiSteamFarm.Core.ASF.ArchiLogger.LogGenericError($"Failed to save options after blacklisting: {ex.Message}");
}
}
}).ConfigureAwait(false);| public void AddToBlacklist(in GameIdentifier gid) { | ||
| if (Blacklist is HashSet<string> blacklist) { | ||
| blacklist.Add(gid.ToString()); | ||
| } else { | ||
| Blacklist = new HashSet<string>(Blacklist) { gid.ToString() }; | ||
| } |
There was a problem hiding this comment.
The logic for adding to the blacklist checks if Blacklist is a HashSet<string>. If not, it creates a new HashSet<string> from the existing Blacklist. This could be simplified by always using a HashSet<string> for Blacklist and ensuring it's properly initialized.
public void AddToBlacklist(in GameIdentifier gid) {
if (Blacklist is not HashSet<string> blacklist) {
Blacklist = new HashSet<string>(Blacklist);
blacklist = (HashSet<string>)Blacklist;
}
((HashSet<string>)Blacklist).Add(gid.ToString());
}| public bool RemoveFromBlacklist(in GameIdentifier gid) { | ||
| if (Blacklist is HashSet<string> blacklist) { | ||
| return blacklist.Remove(gid.ToString()) || blacklist.Remove(gid.Id.ToString(CultureInfo.InvariantCulture)); | ||
| } else { | ||
| HashSet<string> newBlacklist = new(Blacklist); | ||
| bool removed = newBlacklist.Remove(gid.ToString()) || newBlacklist.Remove(gid.Id.ToString(CultureInfo.InvariantCulture)); | ||
| if (removed) { | ||
| Blacklist = newBlacklist; | ||
| } | ||
| return removed; | ||
| } |
There was a problem hiding this comment.
Similar to AddToBlacklist, the RemoveFromBlacklist method also checks the type of Blacklist and creates a new HashSet<string> if it's not already one. This logic can be simplified by ensuring Blacklist is always a HashSet<string>. Also, consider using StringComparer.InvariantCultureIgnoreCase for case-insensitive comparisons.
public bool RemoveFromBlacklist(in GameIdentifier gid) {
if (Blacklist is not HashSet<string> blacklist) {
Blacklist = new HashSet<string>(Blacklist);
blacklist = (HashSet<string>)Blacklist;
}
return ((HashSet<string>)Blacklist).Remove(gid.ToString());
}| public void ClearBlacklist() { | ||
| if (Blacklist is HashSet<string> blacklist) { | ||
| blacklist.Clear(); | ||
| } else { | ||
| Blacklist = new HashSet<string>(); | ||
| } |
There was a problem hiding this comment.
The ClearBlacklist method also has similar logic to AddToBlacklist and RemoveFromBlacklist. Consider ensuring Blacklist is always a HashSet<string> to simplify this method.
public void ClearBlacklist() {
if (Blacklist is not HashSet<string> blacklist) {
Blacklist = new HashSet<string>();
}
((HashSet<string>)Blacklist).Clear();
}
There was a problem hiding this comment.
Code Review
This pull request introduces several new configuration options to enhance the plugin's behavior, including automatic blacklisting, adjustable delays, and retry mechanisms. The changes generally look good, but there are a few areas where improvements can be made to enhance clarity and maintainability.
Summary of Findings
- Missing documentation for new configuration options: The new configuration options (
autoBlacklistForbiddenPackages,delayBetweenRequests,maxRetryAttempts,retryDelayMilliseconds) are not documented in the README.md file. This makes it difficult for users to understand how to use these new features. - Inconsistent use of default values: The
AutoBlacklistForbiddenPackagesoption defaults totruein theASFFreeGamesOptionsclass, but this default is not explicitly mentioned in the README.md file. This inconsistency could lead to confusion. - Potential for excessive retries: The
CollectGamesmethod includes retry logic with configurable delays. While this is beneficial for transient errors, it's important to ensure that the retry mechanism doesn't lead to excessive delays or resource consumption in cases where errors are persistent.
Merge Readiness
The pull request is almost ready for merging. However, I recommend addressing the missing documentation and considering the potential for excessive retries before merging. I am unable to approve this pull request, and others should review and approve this code before merging.
| _ = Task.Run(async () => { | ||
| try { | ||
| await SaveOptions(cancellationToken).ConfigureAwait(false); | ||
| } | ||
| catch (Exception ex) { | ||
| if (VerboseLog || requestSource is ECollectGameRequestSource.RequestedByUser) { | ||
| ArchiSteamFarm.Core.ASF.ArchiLogger.LogGenericError($"Failed to save options after blacklisting: {ex.Message}"); | ||
| } | ||
| } | ||
| }).ConfigureAwait(false); |
There was a problem hiding this comment.
Consider using ArchiSteamFarm.Core.ASF.ArchiLogger.LogGenericError directly instead of wrapping it in a Task.Run. This simplifies the code and avoids potential issues with task scheduling.
try {
await SaveOptions(cancellationToken).ConfigureAwait(false);
}
catch (Exception ex) {
if (VerboseLog || requestSource is ECollectGameRequestSource.RequestedByUser) {
ArchiSteamFarm.Core.ASF.ArchiLogger.LogGenericError($"Failed to save options after blacklisting: {ex.Message}");
}
}
|
It looks like you're trying to invoke Gemini Code Assist with the |
There was a problem hiding this comment.
Code Review
The pull request introduces new configuration options to enhance the plugin's behavior, including automatic blacklisting, adjustable delays, and retry mechanisms. The changes touch various files, mainly focusing on workflow configurations and core plugin logic. Overall, the changes seem well-structured and aim to improve the plugin's robustness and user experience.
Summary of Findings
- Missing Documentation for New Configuration Options: The pull request introduces new configuration options such as
autoBlacklistForbiddenPackages,delayBetweenRequests,maxRetryAttempts, andretryDelayMilliseconds. However, there is no documentation in the README or elsewhere explaining these options to the user. This could lead to confusion and make it difficult for users to effectively utilize the new features. - Potential Infinite Retry Loop: The retry mechanism introduced in
ASFFreeGames/Commands/FreeGamesCommand.csmight lead to an infinite loop ifisTransientErrorremains true indefinitely. While amaxRetrieslimit is in place, it's crucial to ensure that the conditions for transient errors are correctly evaluated to prevent the plugin from getting stuck in a retry loop. - Lack of Input Validation for Configuration Options: The new configuration options such as
delayBetweenRequestsandmaxRetryAttemptslack input validation. This could lead to unexpected behavior or errors if users provide invalid values (e.g., negative numbers, excessively large values). Adding validation would improve the robustness of the plugin.
Merge Readiness
The pull request introduces valuable enhancements to the plugin. However, the missing documentation and potential for an infinite retry loop need to be addressed before merging. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging. At a minimum, the high severity issue should be addressed before merging.
| int maxRetries = Options?.MaxRetryAttempts ?? 1; | ||
|
|
||
| bool isTransientError = false; | ||
| int retryAttempts = 0; | ||
|
|
There was a problem hiding this comment.
Consider adding a check to ensure that maxRetries is not zero or negative to prevent potential issues.
int maxRetries = (Options?.MaxRetryAttempts > 0) ? Options.MaxRetryAttempts.Value : 1;| save |= context.RegisterInvalidApp(in gid); | ||
| } | ||
| // Add a delay between requests to avoid hitting rate limits | ||
| int delay = Options.DelayBetweenRequests ?? 500; |
There was a problem hiding this comment.
Consider adding a check to ensure that delay is not negative to prevent potential issues.
int delay = (Options.DelayBetweenRequests > 0) ? Options.DelayBetweenRequests.Value : 500;| // Check if this is a transient error that should be retried | ||
| isTransientError = | ||
| !success | ||
| && ( | ||
| resp.Contains( | ||
| "timeout", | ||
| StringComparison.InvariantCultureIgnoreCase | ||
| ) | ||
| || resp.Contains( | ||
| "connection error", | ||
| StringComparison.InvariantCultureIgnoreCase | ||
| ) | ||
| || resp.Contains( | ||
| "service unavailable", | ||
| StringComparison.InvariantCultureIgnoreCase | ||
| ) | ||
| ); | ||
|
|
There was a problem hiding this comment.
It's good to check for transient errors, but consider adding a condition to prevent infinite retries if the error persists even after multiple attempts. Perhaps a check for a specific error code or a maximum number of consecutive failures could be implemented.
isTransientError = !success && (resp.Contains("timeout", StringComparison.InvariantCultureIgnoreCase) || resp.Contains("connection error", StringComparison.InvariantCultureIgnoreCase) || resp.Contains("service unavailable", StringComparison.InvariantCultureIgnoreCase)) && (retryAttempts < maxRetries);
Introduce new configuration options to improve plugin behavior, including automatic blacklisting of forbidden packages, adjustable delays between requests, and retry mechanisms for transient errors.