新增基于状态机模型的规则模块 #160
新增基于状态机模型的规则模块 #160
Conversation
2、将规则订阅和Peer黑名单的匹配模块基于新的规则规则模块进行重构 3、微调规则订阅模块方便后续加入更多订阅规则的匹配类型
…NTAINS/CLIENT_NAME_STARTS_WITH
WalkthroughThe changes introduced a new state machine-based banning mechanism by adding multiple classes and modifying existing ones to manage peer connections and bans more efficiently. Key additions include constructing state machines, updating peer conditions, and enhanced rule matching. The updated logic significantly improves how peer bans are managed, focusing on modular rule handling and better configuration. Changes
Sequence DiagramsNew State Machine-Based Peer Ban WorkflowsequenceDiagram
actor Admin
participant PeerBanHelperServer
participant RuleBlocker
participant StateMachine
participant PeerMatchRecord
Admin ->> PeerBanHelperServer: Initialize Server
PeerBanHelperServer ->> RuleBlocker: registerModules()
RuleBlocker ->> StateMachine: Initialize StateMachine
RuleBlocker ->> PeerMatchRecord: Create matchRecords map
PeerMatchRecord ->> StateMachine: Evaluate match with rules
StateMachine ->> PeerMatchRecord: Update state based on events
RuleBlocker ->> PeerBanHelperServer: Update peer state
PeerBanHelperServer ->> Admin: Inform about ban wave
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Outside diff range and nitpick comments (5)
src/main/java/com/ghostchu/peerbanhelper/module/AbstractRuleBlocker.java (1)
3-17: Ensure proper documentation for public members and methods inAbstractRuleBlocker.Adding Javadoc comments to public members and methods would enhance maintainability and understandability, especially for abstract classes intended to be extended by others.
src/main/java/com/ghostchu/peerbanhelper/module/impl/webapi/RuleSubController.java (2)
106-106: Improve error messaging to include details about what parameter was incorrect.ctx.json(new SlimMsg(false, Lang.SUB_RULE_CHECK_INTERVAL_WRONG_PARAM + ": checkInterval"));
129-131: Include more specific error details in the log message to aid in debugging.log.error(Lang.SUB_RULE_LOG_QUERY_ERROR + " for ruleId: " + ruleId, e);src/main/java/com/ghostchu/peerbanhelper/PeerBanHelperServer.java (2)
Line range hint
381-456: The methodbanWavehas been significantly modified to incorporate state machine logic for ban management. It's important to ensure that the logic here is thoroughly tested, especially since it deals with concurrency and state transitions which are prone to errors. Consider adding more detailed logs or comments explaining each state transition for future maintainability.+ // Consider adding more detailed logs or comments explaining each state transition for future maintainability.
Line range hint
727-757: ThebanPeermethod has been refactored to support the new rule-based blocking logic. It is crucial to ensure that all parameters are correctly passed and that the ban logic is accurately implemented. Additionally, consider optimizing the DNS reverse lookup to avoid potential performance bottlenecks.+ // Consider optimizing the DNS reverse lookup to avoid potential performance bottlenecks.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (22)
- pom.xml (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/PeerBanHelperServer.java (10 hunks)
- src/main/java/com/ghostchu/peerbanhelper/config/ProfileUpdateScript.java (3 hunks)
- src/main/java/com/ghostchu/peerbanhelper/database/DatabaseHelper.java (3 hunks)
- src/main/java/com/ghostchu/peerbanhelper/database/RuleSubLog.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/module/AbstractRuleBlocker.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/module/MatchEvents.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/module/MatchResultDetail.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/module/PeerMatchRecord.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/module/PeerState.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/module/RuleBlocker.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/module/RuleUpdateType.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/module/impl/rule/PeerIdBlocker.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/module/impl/rule/RuleSubBlocker.java (7 hunks)
- src/main/java/com/ghostchu/peerbanhelper/module/impl/webapi/RuleSubController.java (14 hunks)
- src/main/java/com/ghostchu/peerbanhelper/text/Lang.java (3 hunks)
- src/main/java/com/ghostchu/peerbanhelper/util/rule/RuleMatcher.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/util/rule/RuleType.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/util/rule/matcher/IPMatcher.java (2 hunks)
- src/main/java/com/ghostchu/peerbanhelper/util/rule/matcher/PrefixMatcher.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/util/rule/matcher/SubStrMatcher.java (1 hunks)
- src/main/resources/profile.yml (3 hunks)
Files skipped from review due to trivial changes (5)
- pom.xml
- src/main/java/com/ghostchu/peerbanhelper/module/MatchEvents.java
- src/main/java/com/ghostchu/peerbanhelper/module/PeerState.java
- src/main/java/com/ghostchu/peerbanhelper/module/RuleUpdateType.java
- src/main/java/com/ghostchu/peerbanhelper/util/rule/RuleType.java
Additional comments not posted (34)
src/main/java/com/ghostchu/peerbanhelper/module/MatchResultDetail.java (1)
3-4: TheMatchResultDetailrecord is well-defined and appropriately encapsulates the result details of a match operation.src/main/java/com/ghostchu/peerbanhelper/database/RuleSubLog.java (1)
3-9: The refactoring inRuleSubLogto useRuleUpdateTypealigns with the broader support for different rule types as stated in the PR objectives.src/main/java/com/ghostchu/peerbanhelper/module/PeerMatchRecord.java (1)
10-21: ThePeerMatchRecordclass is well-defined, making effective use of Lombok annotations to simplify data class boilerplate. It appropriately encapsulates peer-related data along with match results.src/main/java/com/ghostchu/peerbanhelper/util/rule/RuleMatcher.java (1)
9-33: TheRuleMatcherclass is effectively designed as an abstract base for specific rule matchers. It uses immutability and encapsulation well, aligning with good OOP practices and the needs of a rule-based system.
[APROVED]src/main/java/com/ghostchu/peerbanhelper/module/AbstractRuleBlocker.java (1)
16-16: Verify the thread safety ofStateMachineusage.src/main/java/com/ghostchu/peerbanhelper/module/impl/rule/PeerIdBlocker.java (1)
45-49: Ensure thread safety for web API endpoint registration.src/main/java/com/ghostchu/peerbanhelper/util/rule/matcher/IPMatcher.java (3)
6-7: Import ofRuleMatcherandRuleTypeis aligned with the changes to support new rule types.
22-29: TheIPMatcherconstructor properly initializes with theRuleType.IP, ensuring that the matcher is correctly categorized.
69-69: ThematcherIdentifiermethod now returns a unique identifier for theIPMatcher, which is good for distinguishing this matcher in logs and debug output.src/main/java/com/ghostchu/peerbanhelper/module/RuleBlocker.java (2)
67-74: ThegetServerandgetLoggermethods provide essential components for the rule blocker functionality. Ensuring these are exposed in the interface allows for better modularity and testing.
81-109: The methodsgetStateMachine,shouldBanPeer, andtriggerEventare crucial for the operation of rule-based blocking. The implementation oftriggerEventparticularly ensures that the state machine events are fired correctly, which is central to the functionality of this module.src/main/resources/profile.yml (2)
9-9: Introduction ofdisconnect-durationin the configuration aligns with the new features related to managing disconnect timeouts. This is a necessary addition for the enhanced functionality.
Line range hint
167-178: The configuration forrule-sub-blockersis set correctly according to the new module requirements. However, ensure that the URL provided is valid and accessible.Verification successful
The URL "https://raw.githubusercontent.com/PBH-BTN/BTN-Collected-Rules/main/dt_torrent.txt" is accessible as confirmed by the script output.
- The URL is accessible.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the URL is accessible if curl --output /dev/null --silent --head --fail "https://raw.githubusercontent.com/PBH-BTN/BTN-Collected-Rules/main/dt_torrent.txt"; then echo "URL is accessible." else echo "URL is not accessible." fiLength of output: 177
src/main/java/com/ghostchu/peerbanhelper/config/ProfileUpdateScript.java (1)
22-29: ThesubModuleUpdatemethod handles the transition of configuration fromip-address-blocker-rulestorule-sub-blockerseffectively. This is crucial for the migration to the new rule system.src/main/java/com/ghostchu/peerbanhelper/module/impl/webapi/RuleSubController.java (1)
154-154: Ensure consistent error handling and user feedback across all API endpoints.Also applies to: 158-158, 175-175, 181-181, 206-206, 230-230, 237-237, 243-243, 251-251, 280-280
src/main/java/com/ghostchu/peerbanhelper/text/Lang.java (3)
24-24: Renamed string constant to reflect the new rule subscription system.
172-202: All these changes are related to renaming string constants to align with the new rule subscription system. This is consistent with the PR's objective of refactoring rule modules and expanding rule types.
215-216: Added new string constants for rule module operations, enhancing traceability and debugging capabilities.src/main/java/com/ghostchu/peerbanhelper/module/impl/rule/RuleSubBlocker.java (11)
8-20: Import statements have been added for new classes that are part of the rule system overhaul. This includes matchers for different rule types and utilities for rule handling.
31-31: Added import for Logger, likely used for logging within this class.
42-42: ImportedCountDownLatchand related concurrency utilities, indicating the use of asynchronous programming patterns within this class.
50-50: The class now extendsAbstractRuleBlocker, aligning with the new architectural design of the rule system.
54-54: Added a field for managing the checking interval and its corresponding getter. The constructor has been modified to accept aDatabaseHelperobject, integrating database operations directly into rule handling.Also applies to: 58-58
64-64: Standard methods for module management such asgetName,getConfigName, andgetLogger. These align with the expected modular design patterns.Also applies to: 69-69, 74-74
80-80: MethodsonEnableandinitsetup the rule checking interval and load initial configurations. The use of a state machine is evident here, which is central to the new rule handling logic.Also applies to: 89-98
106-113: ThereloadConfigmethod ensures dynamic updating of the configuration without needing a system restart, enhancing the flexibility of the rule handling system.
115-141: TheshouldBanPeermethod implements the logic to determine if a peer should be banned based on the loaded rules. It uses asynchronous programming patterns to evaluate all rules concurrently, which should improve performance on large rule sets.
Line range hint
149-279: TheupdateRulemethod handles the logic for updating rules from a remote source or local cache. It includes error handling, logging, and database interactions to log rule updates. This method is central to the functionality of the rule module, ensuring rules are current and correctly applied.
284-300: Utility methodsfileToLinesandfileToIPListare used to parse rule files into usable data structures. These methods are well-implemented with error handling and logging, supporting the main functionality of rule parsing.src/main/java/com/ghostchu/peerbanhelper/PeerBanHelperServer.java (5)
74-74: The addition ofdisconnectTimeoutis well-aligned with the PR's objectives to improve ban logic using a state machine. Ensure this value is appropriately documented and validated.
100-100: ChangingmoduleManagertofinalis a good practice as it ensures immutability where possible, which can help prevent accidental reassignments and bugs related to concurrency.
108-108: ThematchRecordsmap is a crucial component for the new state machine logic. It's important to ensure thread safety, which is achieved here by usingConcurrentHashMap. This is particularly vital given the asynchronous nature of the operations that modify this map.
114-114: Proper initialization ofdisconnectTimeoutfrom the configuration file is done here. It's essential to handle potential misconfigurations gracefully, such as providing defaults or logging warnings if the configuration is missing or invalid.
584-593: The registration of modules has been updated to include the newRuleSubBlocker. This change should be reflected across all configurations and documentation to ensure that users are aware of the new capabilities and how to configure them.
src/main/java/com/ghostchu/peerbanhelper/PeerBanHelperServer.java
Outdated
Show resolved
Hide resolved
| } catch (InterruptedException e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
Using a RuntimeException to handle InterruptedException is generally not recommended. It's better to either re-interrupt the thread or handle the interruption more gracefully, depending on the desired application behavior.
- throw new RuntimeException(e);
+ Thread.currentThread().interrupt(); // Restore the interrupted status
// or handle the interruption more appropriately depending on the contextCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (InterruptedException e) { | |
| throw new RuntimeException(e); | |
| } catch (InterruptedException e) { | |
| Thread.currentThread().interrupt(); // Restore the interrupted status | |
| // or handle the interruption more appropriately depending on the context |
There was a problem hiding this comment.
@Ghost-chu, I've noted your decision to apply the suggested changes to handle InterruptedException more appropriately. Please ensure to update the exception handling in your development environment as discussed. If you need any more assistance or further modifications, feel free to ask!
| @Override | ||
| public void setData(String ruleName, Object... ruleData) { | ||
| this.ruleName = ruleName; | ||
| this.ips = ips; | ||
| this.subnets = subnets; | ||
| this.ips = ruleData.length > 0 ? (List<IPAddress>) ruleData[0] : List.of(); | ||
| this.subnets = ruleData.length > 1 ? (List<IPAddress>) ruleData[1] : List.of(); |
There was a problem hiding this comment.
Ensure type safety and potential ClassCastException when casting ruleData to List<IPAddress>.
Consider adding type checks before casting to avoid runtime exceptions:
if (ruleData.length > 0 && ruleData[0] instanceof List) {
this.ips = (List<IPAddress>) ruleData[0];
} else {
this.ips = List.of();
}
// Repeat for subnetsThere was a problem hiding this comment.
@Ghost-chu, 您的建议非常合理。使用泛型可以在编译时检查类型安全，避免运行时的类型转换错误。这里是按照您的建议重构的代码片段：
public class IPMatcher extends RuleMatcher<List<IPAddress>> {
private List<IPAddress> subnets;
private List<IPAddress> ips;
private BloomFilter<String> bloomFilter;
public IPMatcher(String ruleId, String ruleName, List<IPAddress> ruleData) {
super(RuleType.IP, ruleId, ruleName, ruleData);
setData(ruleName, ruleData);
}
@Override
public void setData(String ruleName, List<IPAddress> ruleData) {
this.ruleName = ruleName;
this.ips = ruleData != null ? ruleData : List.of();
this.subnets = ruleData != null ? ruleData : List.of();
bloomFilter = BloomFilter.create(Funnels.stringFunnel(StandardCharsets.UTF_8), ips.size(), 0.01);
ips.forEach(ip -> bloomFilter.put(ip.toString()));
}
@Override
public @NotNull MatchResult match0(@NotNull String content) {
IPAddress pa = IPAddressUtil.getIPAddress(content);
if (pa.isIPv4Convertible()) {
pa = pa.toIPv4();
}
final IPAddress ip = pa;
if (bloomFilter.mightContain(content)) {
if (ips.stream().anyMatch(ele -> ele.isIPv4Convertible() == ip.isIPv4Convertible() && ele.equals(ip))) {
return MatchResult.TRUE;
}
}
if (subnets.stream().anyMatch(subnet -> subnet.contains(ip))) {
return MatchResult.TRUE;
}
return MatchResult.DEFAULT;
}
@Override
public @NotNull String matcherName() {
return Lang.RULE_MATCHER_SUB_RULE;
}
@Override
public String matcherIdentifier() {
return "peerbanhelper:ipmatcher";
}
}这段代码中，IPMatcher 类现在接受一个具体的 List<IPAddress> 类型作为参数，这样可以在编译时确保类型安全。如果有其他需要调整的地方，请告诉我！
src/main/java/com/ghostchu/peerbanhelper/module/RuleBlocker.java
Outdated
Show resolved
Hide resolved
src/main/java/com/ghostchu/peerbanhelper/module/impl/webapi/RuleSubController.java
Show resolved
Hide resolved
| @Override | ||
| public void setData(String ruleName, Object... ruleData) { | ||
| this.ruleName = ruleName; | ||
| this.subStrs = ruleData.length > 0 ? (List<String>) ruleData[0] : List.of(); |
There was a problem hiding this comment.
Refactor setData to handle potential ClassCastException.
When casting ruleData[0] to List<String>, ensure it is safely cast to avoid runtime exceptions.
@Override
public void setData(String ruleName, Object... ruleData) {
this.ruleName = ruleName;
- this.subStrs = ruleData.length > 0 ? (List<String>) ruleData[0] : List.of();
+ this.subStrs = (ruleData.length > 0 && ruleData[0] instanceof List) ? (List<String>) ruleData[0] : List.of();
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public void setData(String ruleName, Object... ruleData) { | |
| this.ruleName = ruleName; | |
| this.subStrs = ruleData.length > 0 ? (List<String>) ruleData[0] : List.of(); | |
| @Override | |
| public void setData(String ruleName, Object... ruleData) { | |
| this.ruleName = ruleName; | |
| this.subStrs = (ruleData.length > 0 && ruleData[0] instanceof List) ? (List<String>) ruleData[0] : List.of(); |
There was a problem hiding this comment.
@Ghost-chu, 您提出的使用泛型来避免不安全的类型转换是一个很好的建议。这样可以在编译时就确保类型安全，减少运行时的错误。下面是根据您的建议重构的代码：
package com.ghostchu.peerbanhelper.util.rule.matcher;
import com.ghostchu.peerbanhelper.text.Lang;
import com.ghostchu.peerbanhelper.util.rule.MatchResult;
import com.ghostchu.peerbanhelper.util.rule.RuleMatcher;
import com.ghostchu.peerbanhelper.util.rule.RuleType;
import lombok.EqualsAndHashCode;
import lombok.ToString;
import lombok.extern.slf4j.Slf4j;
import org.jetbrains.annotations.NotNull;
import java.util.List;
@Slf4j
@EqualsAndHashCode(callSuper = true)
@ToString(callSuper = true)
public class SubStrMatcher<T extends List<String>> extends RuleMatcher {
private T subStrs;
public SubStrMatcher(RuleType ruleType, String ruleId, String ruleName, Object... ruleData) {
super(ruleType, ruleId, ruleName, ruleData);
}
@Override
public void setData(String ruleName, T ruleData) {
this.ruleName = ruleName;
this.subStrs = ruleData != null ? ruleData : List.of();
}
}在这个版本中，SubStrMatcher 类现在是泛型的，它期望其数据是 List<String> 类型。这样，任何不符合此类型的数据都会在编译时被捕获，而不是在运行时抛出 ClassCastException。如果有其他问题或需要进一步的调整，请告诉我！
| if (subStrs.parallelStream().anyMatch(content::contains)) { | ||
| return MatchResult.TRUE; |
There was a problem hiding this comment.
Optimize parallel stream usage in match0.
Using parallelStream for potentially small collections might not be efficient due to the overhead of thread management. Consider conditions under which parallelism is beneficial.
@Override
public @NotNull MatchResult match0(@NotNull String content) {
- if (subStrs.parallelStream().anyMatch(content::contains)) {
+ if (subStrs.stream().anyMatch(content::contains)) {
return MatchResult.TRUE;
}
return MatchResult.DEFAULT;
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (subStrs.parallelStream().anyMatch(content::contains)) { | |
| return MatchResult.TRUE; | |
| @Override | |
| public @NotNull MatchResult match0(@NotNull String content) { | |
| if (subStrs.stream().anyMatch(content::contains)) { | |
| return MatchResult.TRUE; | |
| } | |
| return MatchResult.DEFAULT; | |
| } |
There was a problem hiding this comment.
避免使用 parallelStream() 除非有十分耗时且大量的操作，否则上下文切换和线程调度开销反而会使得性能变差。
stream 本身也比较费时，JVM 并不会对 stream 进行太多编译优化，对于十分简单的操作，依然建议使用传统 for 循环。
There was a problem hiding this comment.
@Ghost-chu, 您的建议非常中肯。对于 SubStrMatcher 类中的 match0 方法，确实应该优先考虑性能和简洁性。考虑到 subStrs 可能只包含少量元素，使用传统的 for 循环将是更优的选择，这样可以避免 stream 引入的额外开销。以下是修改后的代码示例：
@Override
public @NotNull MatchResult match0(@NotNull String content) {
for (String subStr : subStrs) {
if (content.contains(subStr)) {
return MatchResult.TRUE;
}
}
return MatchResult.DEFAULT;
}这段代码直接遍历 subStrs 列表，并检查内容中是否包含任何子字符串。如果找到匹配项，立即返回 MatchResult.TRUE；如果遍历完列表没有找到，返回 MatchResult.DEFAULT。这种方式比使用 stream 更直接且效率更高。
请您查看上述修改是否符合预期，如果有其他建议或问题，欢迎继续讨论。
| if (prefixes.parallelStream().anyMatch(content::startsWith)) { | ||
| return MatchResult.TRUE; |
There was a problem hiding this comment.
Optimize parallel stream usage in match0.
Using parallelStream for potentially small collections might not be efficient due to the overhead of thread management. Consider conditions under which parallelism is beneficial.
@Override
public @NotNull MatchResult match0(@NotNull String content) {
- if (prefixes.parallelStream().anyMatch(content::startsWith)) {
+ if (prefixes.stream().anyMatch(content::startsWith)) {
return MatchResult.TRUE;
}
return MatchResult.DEFAULT;
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (prefixes.parallelStream().anyMatch(content::startsWith)) { | |
| return MatchResult.TRUE; | |
| if (prefixes.stream().anyMatch(content::startsWith)) { | |
| return MatchResult.TRUE; |
There was a problem hiding this comment.
避免使用 parallelStream() 除非有十分耗时且大量的操作，否则上下文切换和线程调度开销反而会使得性能变差。
stream 本身也比较费时，JVM 并不会对 stream 进行太多编译优化，对于十分简单的操作，依然建议使用传统 for 循环。
There was a problem hiding this comment.
@Ghost-chu, 您的建议非常中肯！确实，对于简单的字符串前缀匹配操作，使用 parallelStream 可能会由于线程管理和上下文切换的开销而导致性能不佳。我同意您的观点，对于这种情况，传统的 for 循环可能会更加高效。
这里是按照您的建议修改后的代码：
@Override
public @NotNull MatchResult match0(@NotNull String content) {
for (String prefix : prefixes) {
if (content.startsWith(prefix)) {
return MatchResult.TRUE;
}
}
return MatchResult.DEFAULT;
}这种方式避免了使用 stream 带来的额外开销，应该能够提升方法的执行效率。如果还有其他需要讨论或优化的地方，请随时告诉我！
Learnings added
User: Ghost-chu
PR: PBH-BTN/PeerBanHelper#160
File: src/main/java/com/ghostchu/peerbanhelper/util/rule/matcher/PrefixMatcher.java:33-34
Timestamp: 2024-06-20T05:54:47.669Z
Learning: Avoid using parallelStream() for simple operations due to the overhead of thread management and context switching. Traditional for-loops are recommended for simple string matching tasks.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure review instructions.
| @Override | ||
| public void setData(String ruleName, Object... ruleData) { | ||
| this.ruleName = ruleName; | ||
| this.prefixes = ruleData.length > 0 ? (List<String>) ruleData[0] : List.of(); |
There was a problem hiding this comment.
Refactor setData to handle potential ClassCastException.
When casting ruleData[0] to List<String>, ensure it is safely cast to avoid runtime exceptions.
@Override
public void setData(String ruleName, Object... ruleData) {
this.ruleName = ruleName;
- this.prefixes = ruleData.length > 0 ? (List<String>) ruleData[0] : List.of();
+ this.prefixes = (ruleData.length > 0 && ruleData[0] instanceof List) ? (List<String>) ruleData[0] : List.of();
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public void setData(String ruleName, Object... ruleData) { | |
| this.ruleName = ruleName; | |
| this.prefixes = ruleData.length > 0 ? (List<String>) ruleData[0] : List.of(); | |
| @Override | |
| public void setData(String ruleName, Object... ruleData) { | |
| this.ruleName = ruleName; | |
| this.prefixes = (ruleData.length > 0 && ruleData[0] instanceof List) ? (List<String>) ruleData[0] : List.of(); |
There was a problem hiding this comment.
@Ghost-chu, 您的建议非常合理。通过使用泛型，我们可以在编译时确保类型安全，避免运行时的类型转换错误。以下是根据您的建议重构的 setData 方法：
@Override
public void setData(String ruleName, List<String> prefixes) {
this.ruleName = ruleName;
this.prefixes = prefixes != null ? prefixes : List.of();
}这样修改后，PrefixMatcher 类的使用者需要在调用 setData 方法时提供 List<String> 类型的参数，这确保了类型的安全性和一致性。如果有其他相关的类或方法也需要调整，请告知，我可以帮助进一步修改。
src/main/java/com/ghostchu/peerbanhelper/module/impl/rule/PeerIdBlocker.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/main/java/com/ghostchu/peerbanhelper/module/impl/rule/RuleSubBlocker.java (7 hunks)
- src/main/java/com/ghostchu/peerbanhelper/util/rule/RuleMatcher.java (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/ghostchu/peerbanhelper/util/rule/RuleMatcher.java
Additional comments not posted (6)
src/main/java/com/ghostchu/peerbanhelper/module/impl/rule/RuleSubBlocker.java (6)
8-10: Imports forAbstractRuleBlocker,PeerMatchRecord, andRuleUpdateTypehave been added. Ensure that these classes/interfaces are correctly implemented and used in the system as expected.
16-20: Addition of imports for various matchers (IPMatcher,PrefixMatcher,SubStrMatcher) aligns with the expanded rule types mentioned in the PR. Ensure these classes are well-implemented and tested.
50-58: TheRuleSubBlockerclass extendsAbstractRuleBlocker. This design promotes reusability and modularity in handling rule-based actions. Confirm that all necessary methods fromAbstractRuleBlockerare appropriately overridden.
64-70: The methodsgetName()andgetConfigName()provide clear identifiers for the module, which is good practice for clarity and maintainability.
292-300: The utility methodfileToLinesis well-implemented for reading files into a list. Ensure that it is used in a context where exception handling is adequately managed.
Line range hint
149-269: TheupdateRulemethod handles rule updates with remote fetching and local caching. It's a critical method with many branching paths and external interactions. Ensure thorough testing, particularly for error handling and edge cases.Consider breaking down this method into smaller, more manageable functions to improve readability and maintainability.
Verification successful
The examination of the
updateRulemethod's usage inRuleSubControllerandBtnAbilityRulesfiles reveals the following:
- In
RuleSubController, theupdateRulemethod is called with proper error handling and logging. For instance, if theConfigurationSectionis null, it returns an error message. Additionally, the results ofupdateRuleare checked, and appropriate actions are taken based on the success or failure of the operation.- In
BtnAbilityRules, theupdateRulemethod is scheduled to run at fixed intervals. However, the actual error handling or logging within theupdateRulemethod itself isn't shown here, but it's implied thatupdateRulehandles its own exceptions and logs errors internally.Given this context, the
updateRulemethod appears to be used correctly with adequate error handling and logging.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `updateRule` method's interaction with external services and file handling. # Test: Search for the method usage. Expect: Proper error handling and logging. rg --type java $'updateRule'Length of output: 1185
Script:
#!/bin/bash # Extract lines around updateRule method usage in RuleSubController.java rg -A 5 -B 5 'updateRule' src/main/java/com/ghostchu/peerbanhelper/module/impl/webapi/RuleSubController.java # Extract lines around updateRule method usage in BtnAbilityRules.java rg -A 5 -B 5 'updateRule' src/main/java/com/ghostchu/peerbanhelper/btn/ability/BtnAbilityRules.javaLength of output: 2260
| } | ||
|
|
||
| @Override | ||
| public void onEnable() { | ||
| stateMachine = ruleSmBuilder().build(getConfigName()); |
There was a problem hiding this comment.
Initialization logic in onEnable() and init() methods is comprehensive. However, consider adding error handling for potential exceptions during configuration loading and rule updates.
+ try {
stateMachine = ruleSmBuilder().build(getConfigName());
ConfigurationSection config = getConfig();
checkInterval = config.getLong("check-interval", checkInterval);
scheduledExecutorService = Executors.newScheduledThreadPool(1);
scheduledExecutorService.scheduleAtFixedRate(this::reloadConfig, 0, checkInterval, TimeUnit.MILLISECONDS);
+ } catch (Exception e) {
+ log.error("Initialization error", e);
+ }Also applies to: 89-98
| public CheckResult shouldBanPeer(PeerMatchRecord ctx) { | ||
| AtomicReference<CheckResult> result = new AtomicReference<>(new CheckResult(false, null, null)); | ||
| CountDownLatch latch = new CountDownLatch(rules.size()); | ||
| try (var service = Executors.newVirtualThreadPerTaskExecutor()) { | ||
| rules.forEach(matcher -> service.submit(() -> { | ||
| String matchStr; | ||
| Object type = matcher.metadata().get("type"); | ||
| if (type == RuleType.PEER_ID_STARTS_WITH || type == RuleType.PEER_ID_CONTAINS) { | ||
| matchStr = ctx.getPeer().getPeerId(); | ||
| } else if (type == RuleType.CLIENT_NAME_STARTS_WITH || type == RuleType.CLIENT_NAME_CONTAINS) { | ||
| matchStr = ctx.getPeer().getClientName(); | ||
| } else { | ||
| matchStr = ctx.getPeer().getAddress().getIp(); | ||
| } | ||
| if (matcher.match(matchStr) == MatchResult.TRUE) { | ||
| result.set(new CheckResult(true, matcher.metadata().get("rule").toString(), String.format(Lang.MODULE_IBL_MATCH_SUB_RULE, matcher.metadata().get("rule").toString()))); | ||
| } | ||
| latch.countDown(); | ||
| })); | ||
| } | ||
| try { | ||
| latch.await(); | ||
| } catch (InterruptedException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| return result.get(); |
There was a problem hiding this comment.
The method shouldBanPeer uses a concurrent approach to evaluate rules, which is efficient. However, the use of AtomicReference and CountDownLatch adds complexity. Consider simplifying this logic or ensuring that it is well-documented for maintainability.
There was a problem hiding this comment.
不需要使用 CountDownLatch，在 try-with-resource 中使用 Executors.newVirtualThreadPerTaskExecutor() 时，离开 try-with-resource 块时会调用 close 方法，其会调用 shutdown 方法。shutdown 方法将等待所有任务完成。
There was a problem hiding this comment.
@Ghost-chu, you're correct about the Executors.newVirtualThreadPerTaskExecutor() behavior in a try-with-resources block. Removing the CountDownLatch can indeed simplify the code by relying on the executor's automatic shutdown to wait for all tasks to complete.
However, it's essential to ensure that the results of all rule checks are correctly aggregated before making a decision on whether to ban a peer. The existing AtomicReference and CountDownLatch mechanism explicitly handles this aggregation. If we remove CountDownLatch, we'll need to ensure that an alternative method for result aggregation is in place to maintain the correctness of the shouldBanPeer method.
Would you like to proceed with refactoring this part to remove CountDownLatch and adjust the result aggregation accordingly? I can help draft the changes if needed.
Learnings added
User: Ghost-chu
PR: PBH-BTN/PeerBanHelper#160
File: src/main/java/com/ghostchu/peerbanhelper/module/impl/rule/RuleSubBlocker.java:116-141
Timestamp: 2024-06-20T05:54:49.474Z
Learning: The `Executors.newVirtualThreadPerTaskExecutor()` used in a try-with-resources statement automatically calls `shutdown()` upon exiting the block, which waits for all tasks to complete.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure review instructions.
|
这可真是个大的变更，能不能多描述一下状态机的实现
…----------
该邮件从移动设备发送
--------------原始邮件--------------
发件人："PluieM ***@***.***>;
发送时间：2024年6月19日(星期三) 晚上9:04
收件人："PBH-BTN/PeerBanHelper" ***@***.***>;
抄送："高渐离 ***@***.***>;"Mention ***@***.***>;
主题：[PBH-BTN/PeerBanHelper] 新增基于状态机模型的规则模块 (PR #160)
-----------------------------------
主要有两点更新
1、按照 #156 中 @Gaojianli 提供的思路 新增基于状态机模型的规则模块，并基于这个模块重构了订阅规则模块和PeerID黑名单模块
2、#155 拓展规则订阅支持的规则类型，现在支持如下几种类型IP/PEER_ID_CONTAINS/PEER_ID_STARTS_WITH/CLIENT_NAME_CONTAINS/CLIENT_NAME_STARTS_WITH
You can view, comment on, or merge this pull request online at:
#160
Commit Summary
2f7f213 1、新增一个基于状态机模型的规则模块
b0cce39 规则订阅支持订阅以下匹配类型 IP/PEER_ID_CONTAINS/PEER_ID_STARTS_WITH/CLIENT_NAME_CONTAINS/CLIENT_NAME_STARTS_WITH
File Changes
(22 files)
M pom.xml (5)
M src/main/java/com/ghostchu/peerbanhelper/PeerBanHelperServer.java (109)
M src/main/java/com/ghostchu/peerbanhelper/config/ProfileUpdateScript.java (11)
M src/main/java/com/ghostchu/peerbanhelper/database/DatabaseHelper.java (6)
M src/main/java/com/ghostchu/peerbanhelper/database/RuleSubLog.java (4)
A src/main/java/com/ghostchu/peerbanhelper/module/AbstractRuleBlocker.java (43)
A src/main/java/com/ghostchu/peerbanhelper/module/MatchEvents.java (27)
A src/main/java/com/ghostchu/peerbanhelper/module/MatchResultDetail.java (5)
A src/main/java/com/ghostchu/peerbanhelper/module/PeerMatchRecord.java (21)
A src/main/java/com/ghostchu/peerbanhelper/module/PeerState.java (27)
A src/main/java/com/ghostchu/peerbanhelper/module/RuleBlocker.java (115)
R src/main/java/com/ghostchu/peerbanhelper/module/RuleUpdateType.java (2)
A src/main/java/com/ghostchu/peerbanhelper/module/impl/rule/PeerIdBlocker.java (65)
R src/main/java/com/ghostchu/peerbanhelper/module/impl/rule/RuleSubBlocker.java (248)
M src/main/java/com/ghostchu/peerbanhelper/module/impl/webapi/RuleSubController.java (90)
M src/main/java/com/ghostchu/peerbanhelper/text/Lang.java (59)
A src/main/java/com/ghostchu/peerbanhelper/util/rule/RuleMatcher.java (35)
A src/main/java/com/ghostchu/peerbanhelper/util/rule/RuleType.java (27)
R src/main/java/com/ghostchu/peerbanhelper/util/rule/matcher/IPMatcher.java (31)
A src/main/java/com/ghostchu/peerbanhelper/util/rule/matcher/PrefixMatcher.java (48)
A src/main/java/com/ghostchu/peerbanhelper/util/rule/matcher/SubStrMatcher.java (48)
M src/main/resources/profile.yml (6)
Patch Links:
https://github.com/PBH-BTN/PeerBanHelper/pull/160.patch
https://github.com/PBH-BTN/PeerBanHelper/pull/160.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
PBH 对于 ClientName、PeerID 和 IP 匹配（本地）都有缓存机制，检查只会执行一次，我在考虑这个状态机是否放在其它地方会好一点。 |
|
状态机的优点是近似检测，比如如果不命中XL规则，那么XL0019就不用看了
…----------
该邮件从移动设备发送
--------------原始邮件--------------
发件人："Ghost_chu ***@***.***>;
发送时间：2024年6月20日(星期四) 中午1:19
收件人："PBH-BTN/PeerBanHelper" ***@***.***>;
抄送："高渐离 ***@***.***>;"Mention ***@***.***>;
主题：Re: [PBH-BTN/PeerBanHelper] 新增基于状态机模型的规则模块 (PR #160)
-----------------------------------
PBH 对于 ClientName、PeerID 和 IP 匹配（本地）都有缓存机制，检查只会执行一次，我在考虑这个状态机是否放在其它地方会好一点。
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
| if (subStrs.parallelStream().anyMatch(content::contains)) { | ||
| return MatchResult.TRUE; |
There was a problem hiding this comment.
避免使用 parallelStream() 除非有十分耗时且大量的操作，否则上下文切换和线程调度开销反而会使得性能变差。
stream 本身也比较费时，JVM 并不会对 stream 进行太多编译优化，对于十分简单的操作，依然建议使用传统 for 循环。
| if (prefixes.parallelStream().anyMatch(content::startsWith)) { | ||
| return MatchResult.TRUE; |
There was a problem hiding this comment.
避免使用 parallelStream() 除非有十分耗时且大量的操作，否则上下文切换和线程调度开销反而会使得性能变差。
stream 本身也比较费时，JVM 并不会对 stream 进行太多编译优化，对于十分简单的操作，依然建议使用传统 for 循环。
| public abstract void setData(String ruleName, Object... ruleData); | ||
|
|
||
| public Map<String, Object> metadata() { | ||
| return Map.of("id", ruleId, "rule", ruleName, "type", ruleType); |
There was a problem hiding this comment.
metadata 应当包含规则内容，以允许用户在 WebUI 等其它位置识别具体触发了哪条规则。
| } | ||
| ctx.status(HttpStatus.BAD_REQUEST); | ||
| ctx.json(new SlimMsg(false, Lang.IP_BAN_RULE_URL_WRONG.replace("{}", ruleName))); | ||
| ctx.json(new SlimMsg(false, Lang.SUB_RULE_URL_WRONG.replace("{}", ruleName))); |
There was a problem hiding this comment.
非日志场景推荐使用 String.format()，并将 {} 更换为 %s
| @@ -386,6 +390,43 @@ public void banWave() { | |||
| // 更新 LIVE_PEERS 用于数据展示 | |||
| banWaveWatchDog.setLastOperation("Update live peers"); | |||
| executor.submit(() -> updateLivePeers(peers)); | |||
| // ===============基于 状态机 的封禁逻辑 | |||
| public @NotNull String getConfigName() { | ||
| return "ip-address-blocker-rules"; | ||
| public Logger getLogger() { | ||
| return log; |
| String msg = Lang.IP_BAN_RULE_DELETED.replace("{}", ruleSubInfo.ruleName()); | ||
| ruleSubBlocker.deleteRuleSubInfo(ruleId); | ||
| ruleSubBlocker.getRules().removeIf(ele -> ele.metadata().get("id").equals(ruleId)); | ||
| String msg = Lang.SUB_RULE_DELETED.replace("{}", ruleSubInfo.ruleName()); |
There was a problem hiding this comment.
同样，建议使用 %s 和 String.format
| if (isAdd && ruleSubInfo != null) { | ||
| // 新增时检查规则是否存在 | ||
| ctx.status(HttpStatus.BAD_REQUEST); | ||
| ctx.json(new SlimMsg(false, Lang.IP_BAN_RULE_ID_CONFLICT.replace("{}", ruleId))); | ||
| ctx.json(new SlimMsg(false, Lang.SUB_RULE_ID_CONFLICT.replace("{}", ruleId))); |
There was a problem hiding this comment.
同样，建议使用 %s 和 String.format
| return; | ||
| } | ||
| if (!isAdd && ruleSubInfo == null) { | ||
| // 更新时检查规则是否存在 | ||
| ctx.status(HttpStatus.BAD_REQUEST); | ||
| ctx.json(new SlimMsg(false, Lang.IP_BAN_RULE_CANT_FIND.replace("{}", ruleId))); | ||
| ctx.json(new SlimMsg(false, Lang.SUB_RULE_CANT_FIND.replace("{}", ruleId))); |
There was a problem hiding this comment.
同样，建议使用 %s 和 String.format
| if (null == configurationSection) { | ||
| return new SlimMsg(false, Lang.IP_BAN_RULE_CANT_FIND.replace("{}", ruleId)); | ||
| return new SlimMsg(false, Lang.SUB_RULE_CANT_FIND.replace("{}", ruleId)); |
There was a problem hiding this comment.
同样，建议使用 %s 和 String.format，太多了 就不一一添加 comment 了
|
@PluieM 简单看了下代码，前缀树的构造在哪个部分呀，没找到呀 |
|
还没调整完毕，容我稍后画图解释 |
|
@coderabbitai 详细解释此PR，如果可以的话，请再画一个图 |
|
Tip For best results, initiate chat on the files or code changes. @Ghost-chu, here's a detailed explanation of the changes introduced in PR #160, along with a diagram to visually represent the new architecture. Detailed ExplanationThis PR introduces a significant update to the PeerBanHelper project, focusing on enhancing the rule-based peer banning system through a state machine model and expanding the rule types supported by the system.
Architectural DiagramBelow is a diagram illustrating the integration of the state machine model with the rule modules: flowchart TD
A[PeerBanHelperServer] -->|Manages| B[AbstractRuleBlocker]
B -->|Utilizes| C[StateMachine]
C --> D[PeerState]
D --> E[MatchEvents]
B -->|Controls| F[RuleBlocker]
F -->|Implements| G[PeerIdBlocker]
F -->|Implements| H[RuleSubBlocker]
C -->|Triggers| I[MatchResultDetail]
I --> J[PeerMatchRecord]
This diagram shows how I hope this provides a clear understanding of the changes and the architecture introduced in this PR. If you have any more questions or need further clarification, feel free to ask! |
|
@coderabbitai 详细解释此PR，如果可以的话，请再画一个图 |
|
@coderabbitai 请详细描述一下 Peer 在此 PR 是如何匹配的。简述各种情况下状态机发生的变化。 |
|
Tip For best results, initiate chat on the files or code changes. @Ghost-chu, as requested, here's a more detailed explanation of the changes introduced in PR #160, along with a diagram to visually represent the new architecture. Detailed ExplanationThis PR introduces a significant update to the PeerBanHelper project, focusing on enhancing the rule-based peer banning system through a state machine model and expanding the rule types supported by the system.
Architectural DiagramBelow is a diagram illustrating the integration of the state machine model with the rule modules: flowchart TD
A[PeerBanHelperServer] -->|Manages| B[AbstractRuleBlocker]
B -->|Utilizes| C[StateMachine]
C --> D[PeerState]
D --> E[MatchEvents]
B -->|Controls| F[RuleBlocker]
F -->|Implements| G[PeerIdBlocker]
F -->|Implements| H[RuleSubBlocker]
C -->|Triggers| I[MatchResultDetail]
I --> J[PeerMatchRecord]
This diagram shows how I hope this provides a clear understanding of the changes and the architecture introduced in this PR. If you have any more questions or need further clarification, feel free to ask! |
|
Tip For best results, initiate chat on the files or code changes. @Ghost-chu, 在此 PR 中，Peer 的匹配和状态机的变化是通过新引入的基于状态机模型的规则模块来管理的。这里是详细的解释： Peer 匹配过程
状态机的变化
这个状态机模型允许系统灵活地处理各种 Peer 状态和事件，确保根据实时数据动态调整 Peer 的处理。如果您需要更多信息或有其他问题，请随时提问！ |
|
@coderabbitai summary |
Actions performedSummary regeneration triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
src/main/java/com/ghostchu/peerbanhelper/PeerBanHelperServer.java (1)
Line range hint
728-760: Review the peer banning logic for potential optimizations.The method
banPeeris crucial for the functionality of banning peers based on various criteria. Consider optimizing the query operations and the conditions under which peers are banned to enhance performance and reduce computational overhead.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/main/java/com/ghostchu/peerbanhelper/PeerBanHelperServer.java (10 hunks)
- src/main/java/com/ghostchu/peerbanhelper/text/Lang.java (3 hunks)
Additional comments not posted (7)
src/main/java/com/ghostchu/peerbanhelper/text/Lang.java (3)
24-24: Ensure new constants align with the application's functionality.The new constants for subscription rules (
MODULE_IBL_MATCH_SUB_RULE) align with the renaming and functional changes in the rest of the application. This should help maintain consistency across the user interface.
172-202: Review the consistency and correctness of the new subscription rule constants.The newly added constants for subscription rule updates and their statuses are comprehensive and cover various scenarios (e.g., success, failure, no update). This should enhance the user experience by providing more detailed feedback about the state of rule updates.
215-216: Confirm the new constants are used correctly across the application.The constants
RULE_MODULE_MATCH_TIMEandRULE_MODULE_PEER_BANare well-defined and seem to be part of the new functionality for rule modules. Ensure these constants are used consistently in logging and user messaging within the rule modules.src/main/java/com/ghostchu/peerbanhelper/PeerBanHelperServer.java (4)
Line range hint
75-117: Ensure proper initialization ofdisconnectTimeoutin the constructor.This change correctly initializes
disconnectTimeoutwith a value from the profile, which is essential for managing peer disconnections based on configured timeouts.
101-110: RefactormoduleManagerto be final and ensure thread safety.Making
moduleManagerfinal is a good practice as it enhances thread-safety and ensures that the reference does not change post-initialization, which is crucial in a multi-threaded environment like this.
518-526: Update match records efficiently and handle potential concurrency issues.The logic for updating
matchRecordsis sound, but make sure to handle concurrency properly, especially sincematchRecordsis aConcurrentHashMap. Consider using atomic operations or locks where necessary to avoid race conditions.
587-596: Ensure new modules are registered correctly and refactor for clarity.The registration of new modules, such as
PeerIdBlockerandRuleSubBlocker, appears correct. However, consider documenting the purpose and functionality of each module during registration to aid future maintainability.
| } catch (InterruptedException e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
Handle InterruptedException properly by restoring the interrupt status.
- throw new RuntimeException(e);
+ Thread.currentThread().interrupt(); // Restore the interrupted status
// or handle the interruption more appropriately depending on the contextInterrupt handling is crucial in a multi-threaded environment to ensure that the thread's interrupt status is respected and that the system can respond appropriately to interrupts.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (InterruptedException e) { | |
| throw new RuntimeException(e); | |
| } catch (InterruptedException e) { | |
| Thread.currentThread().interrupt(); // Restore the interrupted status |
2、精简RuleBlocker的调用逻辑
…r-new # Conflicts: # src/main/java/com/ghostchu/peerbanhelper/PeerBanHelperServer.java # src/main/java/com/ghostchu/peerbanhelper/text/Lang.java
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
src/main/java/com/ghostchu/peerbanhelper/module/impl/rule/RuleSubBlocker.java (1)
Line range hint
48-259:RuleSubBlockereffectively manages rule-based blocking with a robust implementation. Consider enhancing error handling in theinitmethod to replace assert statements with more user-friendly error messages.- assert rule != null; + if (rule == null) { + log.error("Rule configuration section is missing for rule ID: {}", ruleId); + continue; + }src/main/java/com/ghostchu/peerbanhelper/PeerBanHelperServer.java (1)
74-74: Consider adding a comment explaining the purpose ofdisconnectTimeout.This could improve code readability and maintainability, especially for new developers or during future reviews.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (15)
- src/main/java/com/ghostchu/peerbanhelper/PeerBanHelperServer.java (10 hunks)
- src/main/java/com/ghostchu/peerbanhelper/module/AbstractRuleBlocker.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/module/MatchEvents.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/module/MatchResultDetail.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/module/PeerMatchContext.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/module/PeerState.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/module/RuleBlocker.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/module/impl/rule/RuleSubBlocker.java (7 hunks)
- src/main/java/com/ghostchu/peerbanhelper/module/impl/webapi/RuleSubController.java (14 hunks)
- src/main/java/com/ghostchu/peerbanhelper/text/Lang.java (3 hunks)
- src/main/java/com/ghostchu/peerbanhelper/util/rule/RuleMatcher.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/util/rule/matcher/IPMatcher.java (2 hunks)
- src/main/java/com/ghostchu/peerbanhelper/util/rule/matcher/PrefixMatcher.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/util/rule/matcher/SubStrMatcher.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/util/time/ExceptedTime.java (1 hunks)
Files skipped from review as they are similar to previous changes (9)
- src/main/java/com/ghostchu/peerbanhelper/module/AbstractRuleBlocker.java
- src/main/java/com/ghostchu/peerbanhelper/module/MatchEvents.java
- src/main/java/com/ghostchu/peerbanhelper/module/MatchResultDetail.java
- src/main/java/com/ghostchu/peerbanhelper/module/PeerState.java
- src/main/java/com/ghostchu/peerbanhelper/module/RuleBlocker.java
- src/main/java/com/ghostchu/peerbanhelper/module/impl/webapi/RuleSubController.java
- src/main/java/com/ghostchu/peerbanhelper/util/rule/RuleMatcher.java
- src/main/java/com/ghostchu/peerbanhelper/util/rule/matcher/PrefixMatcher.java
- src/main/java/com/ghostchu/peerbanhelper/util/rule/matcher/SubStrMatcher.java
Additional comments not posted (28)
src/main/java/com/ghostchu/peerbanhelper/module/PeerMatchContext.java (1)
1-18: ClassPeerMatchContextis well-structured to handle various peer match actions using functional programming paradigms. Good use of@Data,@AllArgsConstructor, and@NoArgsConstructorannotations for boilerplate code reduction.src/main/java/com/ghostchu/peerbanhelper/util/time/ExceptedTime.java (1)
12-12: The addition ofSTAGE_BAN_WAVEinExceptedTimecentralizes timeout management for related operations, enhancing maintainability and clarity.src/main/java/com/ghostchu/peerbanhelper/util/rule/matcher/IPMatcher.java (1)
Line range hint
24-85:IPMatcheris effectively designed to handle both exact IP and subnet matches with an optimized approach using a bloom filter. Good encapsulation of matching logic.src/main/java/com/ghostchu/peerbanhelper/text/Lang.java (19)
24-24: Updated constant name to reflect change from IP blacklist to subscription rules.
172-172: The constantSUB_RULE_UPDATE_FINISHclearly indicates the completion of rule subscription updates.
173-173: The constantSUB_RULE_NO_UPDATEeffectively communicates that no updates occurred for the specified subscription rule.
174-174: The constantSUB_RULE_UPDATE_SUCCESSaccurately reflects a successful update to a subscription rule.
175-175: The constantSUB_RULE_UPDATE_FAILEDclearly indicates a failure in updating the subscription rule.
176-176: The constantSUB_RULE_LOAD_SUCCESSeffectively communicates the successful loading of a subscription rule.
177-177: The constantSUB_RULE_UPDATE_LOG_ERRORclearly indicates an error occurred during the logging of a subscription rule update.
178-178: The constantSUB_RULE_USE_CACHEeffectively communicates that the local cache was used to successfully load a subscription rule due to a failure in the subscription process.
179-179: The constantSUB_RULE_LOAD_FAILEDclearly indicates a failure in loading a subscription rule.
182-182: The constantSUB_RULE_LOAD_CONTENTclearly indicates the content that was loaded for a subscription rule.
184-184: The constantSUB_RULE_NO_IDeffectively communicates an error due to an empty subscription rule ID.
185-185: The constantSUB_RULE_ID_CONFLICTclearly indicates a conflict in subscription rule IDs.
186-186: The constantSUB_RULE_CANT_FINDeffectively communicates that a specific subscription rule could not be found.
187-187: The constantSUB_RULE_PARAM_WRONGclearly indicates an error due to incorrect parameters for a subscription rule.
188-188: The constantSUB_RULE_URL_WRONGclearly indicates an error due to an incorrect URL for a subscription rule.
189-194: These constants clearly communicate various states and actions related to subscription rules, such as enabling, disabling, updating, saving, and deleting.
195-198: These constants effectively communicate various outcomes of querying subscription rule information and logs.
199-202: These constants clearly communicate various states and actions related to checking and updating the interval for subscription rules.
215-218: These constants clearly communicate various events and states within the rule module, such as match time, peer hit, disconnect, and ban timeout.src/main/java/com/ghostchu/peerbanhelper/PeerBanHelperServer.java (6)
108-108: Initialization ofmatchRecordsappears correct, but ensure thread safety when accessing this map.Consider adding detailed unit tests to verify thread safety and correctness of operations on
matchRecords.
496-496: Ensure that the match records update logic is correct and efficient.Consider adding more comments to explain the logic, especially how match records are managed and updated.
Also applies to: 498-498, 500-500, 502-502, 504-504
566-566: The registration of modules inregisterModulesmethod is updated correctly.Ensure that all newly registered modules are properly integrated and tested with the existing system.
Also applies to: 567-567, 575-575
707-707: ThebanPeermethod handles the new ban logic using the state machine model effectively.Ensure that all edge cases are covered in unit tests, especially with different combinations of downloader, torrent, and peer.
Also applies to: 709-709, 710-710, 712-712, 713-713, 715-715, 717-717, 719-719, 720-720, 722-722, 724-724, 744-744, 745-745
100-100: EnsuremoduleManageris properly initialized before use in constructors or methods.Verification successful
The
moduleManageris properly initialized within the constructor of thePeerBanHelperServerclass before it is used. This ensures that it is correctly set up before any method calls that rely on it.
- Initialization occurs at:
this.moduleManager = new ModuleManager();within the constructor.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for all occurrences of `moduleManager` to ensure it's initialized before use. rg --type java 'moduleManager'Length of output: 3204
Script:
#!/bin/bash # Search for the PeerBanHelperServer class definition and include a broader range to capture the constructor and initialization blocks. rg --type java 'class PeerBanHelperServer' -A 50Length of output: 5483
Script:
#!/bin/bash # Search for the PeerBanHelperServer class definition and extend the range to capture the full constructor. rg --type java 'class PeerBanHelperServer' -A 100Length of output: 10969
114-114: Initialization ofdisconnectTimeoutusing profile settings seems correct. Verify that the corresponding configuration exists and is documented.Verification successful
The
disconnect-timeoutconfiguration is indeed present in the codebase, specifically in theProfileUpdateScript.javafile where it is being set.
src/main/java/com/ghostchu/peerbanhelper/config/ProfileUpdateScript.java:conf.set("disconnect-timeout", 60000);Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of 'disconnect-timeout' in configuration files. rg --type yaml 'disconnect-timeout'Length of output: 35
Script:
#!/bin/bash # Description: Verify the existence of 'disconnect-timeout' in JSON and properties configuration files. rg --type json 'disconnect-timeout' rg --type properties 'disconnect-timeout'Length of output: 261
Script:
#!/bin/bash # Description: Verify the existence of 'disconnect-timeout' in JSON, properties, and conf configuration files. rg 'disconnect-timeout' --glob '*.json' rg 'disconnect-timeout' --glob '*.properties' rg 'disconnect-timeout' --glob '*.conf'Length of output: 572
Script:
#!/bin/bash # Description: Verify the existence of 'disconnect-timeout' in all files within the repository. rg 'disconnect-timeout'Length of output: 281
| @@ -377,7 +378,7 @@ public void banWave() { | |||
| banWaveWatchDog.setLastOperation("Reset last status"); | |||
| // 声明基本集合 | |||
| // 需要重启的种子列表 | |||
| Map<Downloader, Collection<Torrent>> needRelaunched = new ConcurrentHashMap<>(); | |||
| Map<Downloader, List<Torrent>> needRelaunched = new ConcurrentHashMap<>(); | |||
There was a problem hiding this comment.
Refactor the banWave method to improve readability and reduce complexity.
The method is very long and handles multiple responsibilities. Consider breaking it down into smaller, more manageable methods, each handling a specific part of the process.
Also applies to: 393-393, 395-395, 397-397, 399-399, 401-401, 403-403, 405-405, 407-407, 409-409, 427-427, 429-429, 431-431, 433-433, 435-435, 437-437
|
感觉没对齐，能看下qq群吗？ @PluieM 我说的是类似NFA的文法状态机 |
主要有两点更新
1、按照 #156 中 @Gaojianli 提供的思路 新增基于状态机模型的规则模块，并基于这个模块重构了订阅规则模块和PeerID黑名单模块
2、#155 拓展规则订阅支持的规则类型，现在支持如下几种类型IP/PEER_ID_CONTAINS/PEER_ID_STARTS_WITH/CLIENT_NAME_CONTAINS/CLIENT_NAME_STARTS_WITH
Summary by CodeRabbit
New Features
AbstractRuleBlockerand related classes.MatchEvents,MatchResultDetail,PeerMatchContext,PeerState, andRuleBlockerfor comprehensive rule matching.PeerIdBlockerfor blocking peers based on IDs.subModuleUpdate()inProfileUpdateScript.Bug Fixes
Refactor
Style
Lang.javafor improved localization and clarity.