Conversation
pcl-ce-automation
bot
added
🛠️ 等待审查
Reviewer's Guide重构现代版和传统版的 Minecraft Ping 处理逻辑，使其遵循官方分帧协议，增加显式的 ping/pong 延迟测量，并将新的“已解析服务器地址”类型（主机名 + 解析后的 IP + 端口）贯穿到 ping 工厂以及所有 UI 调用方。 现代 Minecraft ping 握手 / 状态 / ping 流程时序图sequenceDiagram
participant UIControl
participant ServerAddressResolver
participant McPingServiceFactory
participant McPingService
participant MinecraftServer
UIControl->>ServerAddressResolver: GetResolvedServerAddressAsync(address)
ServerAddressResolver-->>UIControl: ResolvedServerAddress(host, ip, port)
UIControl->>McPingServiceFactory: CreateService(host, ip, port)
McPingServiceFactory-->>UIControl: McPingService
UIControl->>McPingService: PingAsync(cancellationToken)
activate McPingService
McPingService->>MinecraftServer: TCP_Connect(endpoint)
McPingService->>MinecraftServer: Send_HandshakePacket(host, port)
McPingService->>MinecraftServer: Send_StatusRequestPacket()
McPingService->>MinecraftServer: Send_PingRequestPacket(timestamp)
loop Read_framed_packets
MinecraftServer-->>McPingService: Status_or_Pong_packet
McPingService->>McPingService: _ReadStatusPayloadAsync(stream, cancellationToken)
alt packetId_0_status
McPingService->>McPingService: Store_statusPayload
else packetId_1_pong
McPingService->>McPingService: Compute_latency(now - pongTimestamp)
else unexpected_packet
McPingService->>McPingService: Ignore_packet
end
end
McPingService-->>UIControl: McPingResult(statusJson, latency)
deactivate McPingService
更新后的 McPingService、McPingServiceFactory 与 ServerAddressResolver 类图classDiagram
class McPingService {
-string _host
-IPEndPoint _endpoint
-int _timeout
+McPingService(string ip, int port, int timeout)
+McPingService(string host, IPEndPoint endpoint, int timeout)
+Task~McPingResult~ PingAsync(CancellationToken cancellationToken)
-byte[] _BuildHandshakePacket(string host, int port)
-byte[] _BuildStatusRequestPacket()
-byte[] _BuildPingRequestPacket(long timestamp)
-Task~Tuple~byte[], long~~ _ReadStatusPayloadAsync(Stream stream, CancellationToken cancellationToken)
-long _ReadInt64BigEndian(byte[] data)
-Task~byte[]~ _ReadExactAsync(Stream stream, int length, CancellationToken cancellationToken)
}
class McPingServiceFactory {
+IMcPingService CreateService(string ip, int port, int timeout)
+IMcPingService CreateService(string host, string ip, int port)
+IMcPingService CreateService(string host, string ip, int port, int timeout)
+IMcPingService CreateService(string host, IPEndPoint endpoint)
+IMcPingService CreateService(string host, IPEndPoint endpoint, int timeout)
+IMcPingService CreateLegacyService(string ip, int port, int timeout)
}
class LegacyMcPingService {
-string _ip
-IPEndPoint _endpoint
-int _timeout
+LegacyMcPingService(string ip, int port, int timeout)
+Task~McPingResult~ PingAsync(CancellationToken cancellationToken)
}
class ServerAddressResolver {
<<static>>
+Task~Tuple~string, int~~ GetReachableAddressAsync(string address, CancellationToken cancelToken)
+Task~ResolvedServerAddress~ GetResolvedServerAddressAsync(string address, CancellationToken cancelToken)
}
class ResolvedServerAddress {
+string Host
+string Ip
+int Port
}
McPingServiceFactory ..> McPingService : creates
McPingServiceFactory ..> LegacyMcPingService : creates
ServerAddressResolver ..> ResolvedServerAddress : returns
ServerAddressResolver ..> McPingServiceFactory : used_by_UI_to_construct_ping_service
已解析服务器地址与 ping 服务创建流程图flowchart TD
UserAction["User triggers server ping in UI"] --> Resolve["ServerAddressResolver.GetResolvedServerAddressAsync(address)"]
Resolve --> Resolved["ResolvedServerAddress { Host, Ip, Port }"]
Resolved --> Factory["McPingServiceFactory.CreateService(host, ip, port)"]
Factory --> Service["McPingService(host, IPEndPoint)"]
Service --> Ping["PingAsync(cancellationToken)\nHandshake + Status + Ping"]
Ping --> Result["McPingResult(status, latency)"]
Result --> UIUpdate["UI displays server MOTD, players, and latency"]
文件级变更
与关联 Issue 的对照评估
Tips and commandsInteracting with Sourcery
自定义你的使用体验打开你的 控制台（dashboard），可以：
获取帮助Original review guide in EnglishReviewer's GuideRefactors modern and legacy Minecraft ping handling to follow the official framed protocol, add explicit ping/pong latency measurement, and thread a new resolved-server-address type (host + resolved IP + port) through the ping factory and UI callers. Sequence diagram for modern Minecraft ping handshake/status/ping flowsequenceDiagram
participant UIControl
participant ServerAddressResolver
participant McPingServiceFactory
participant McPingService
participant MinecraftServer
UIControl->>ServerAddressResolver: GetResolvedServerAddressAsync(address)
ServerAddressResolver-->>UIControl: ResolvedServerAddress(host, ip, port)
UIControl->>McPingServiceFactory: CreateService(host, ip, port)
McPingServiceFactory-->>UIControl: McPingService
UIControl->>McPingService: PingAsync(cancellationToken)
activate McPingService
McPingService->>MinecraftServer: TCP_Connect(endpoint)
McPingService->>MinecraftServer: Send_HandshakePacket(host, port)
McPingService->>MinecraftServer: Send_StatusRequestPacket()
McPingService->>MinecraftServer: Send_PingRequestPacket(timestamp)
loop Read_framed_packets
MinecraftServer-->>McPingService: Status_or_Pong_packet
McPingService->>McPingService: _ReadStatusPayloadAsync(stream, cancellationToken)
alt packetId_0_status
McPingService->>McPingService: Store_statusPayload
else packetId_1_pong
McPingService->>McPingService: Compute_latency(now - pongTimestamp)
else unexpected_packet
McPingService->>McPingService: Ignore_packet
end
end
McPingService-->>UIControl: McPingResult(statusJson, latency)
deactivate McPingService
Updated class diagram for McPingService, McPingServiceFactory, and ServerAddressResolverclassDiagram
class McPingService {
-string _host
-IPEndPoint _endpoint
-int _timeout
+McPingService(string ip, int port, int timeout)
+McPingService(string host, IPEndPoint endpoint, int timeout)
+Task~McPingResult~ PingAsync(CancellationToken cancellationToken)
-byte[] _BuildHandshakePacket(string host, int port)
-byte[] _BuildStatusRequestPacket()
-byte[] _BuildPingRequestPacket(long timestamp)
-Task~Tuple~byte[], long~~ _ReadStatusPayloadAsync(Stream stream, CancellationToken cancellationToken)
-long _ReadInt64BigEndian(byte[] data)
-Task~byte[]~ _ReadExactAsync(Stream stream, int length, CancellationToken cancellationToken)
}
class McPingServiceFactory {
+IMcPingService CreateService(string ip, int port, int timeout)
+IMcPingService CreateService(string host, string ip, int port)
+IMcPingService CreateService(string host, string ip, int port, int timeout)
+IMcPingService CreateService(string host, IPEndPoint endpoint)
+IMcPingService CreateService(string host, IPEndPoint endpoint, int timeout)
+IMcPingService CreateLegacyService(string ip, int port, int timeout)
}
class LegacyMcPingService {
-string _ip
-IPEndPoint _endpoint
-int _timeout
+LegacyMcPingService(string ip, int port, int timeout)
+Task~McPingResult~ PingAsync(CancellationToken cancellationToken)
}
class ServerAddressResolver {
<<static>>
+Task~Tuple~string, int~~ GetReachableAddressAsync(string address, CancellationToken cancelToken)
+Task~ResolvedServerAddress~ GetResolvedServerAddressAsync(string address, CancellationToken cancelToken)
}
class ResolvedServerAddress {
+string Host
+string Ip
+int Port
}
McPingServiceFactory ..> McPingService : creates
McPingServiceFactory ..> LegacyMcPingService : creates
ServerAddressResolver ..> ResolvedServerAddress : returns
ServerAddressResolver ..> McPingServiceFactory : used_by_UI_to_construct_ping_service
Flow diagram for resolved server address and ping service creationflowchart TD
UserAction["User triggers server ping in UI"] --> Resolve["ServerAddressResolver.GetResolvedServerAddressAsync(address)"]
Resolve --> Resolved["ResolvedServerAddress { Host, Ip, Port }"]
Resolved --> Factory["McPingServiceFactory.CreateService(host, ip, port)"]
Factory --> Service["McPingService(host, IPEndPoint)"]
Service --> Ping["PingAsync(cancellationToken)\nHandshake + Status + Ping"]
Ping --> Result["McPingResult(status, latency)"]
Result --> UIUpdate["UI displays server MOTD, players, and latency"]
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我发现了 4 个问题，并给出了一些整体性的反馈：
- 在
MinecraftServer.xaml.vb和PageInstanceServer.xaml.vb中，如果Ip包含主机名（这在某些ResolvedServerAddress的回退路径中可能发生），IPAddress.Parse(addr.Ip)会抛异常；可以考虑要么保证Ip始终是实际的 IP 地址，要么使用支持主机名的Dns/IPEndPoint构造方式，而不是使用IPAddress.Parse。 - 在
_ReadStatusPayloadAsync中，循环被硬编码为最多只读取两个数据包；如果服务器发送额外的数据包（例如插件特有的数据包，或顺序不同），你可能希望放宽这个假设（例如循环直到同时拿到 status 和 pong，或直到超时 / EOF），而不是在遇到未知包类型时立刻抛异常。
给 AI Agent 的提示
请根据这次代码审查中的评论进行修改：
## 总体评论
- 在 `MinecraftServer.xaml.vb` 和 `PageInstanceServer.xaml.vb` 中，如果 `Ip` 包含主机名（这在某些 `ResolvedServerAddress` 的回退路径中可能发生），`IPAddress.Parse(addr.Ip)` 会抛异常；可以考虑要么保证 `Ip` 始终是实际的 IP 地址，要么使用支持主机名的 `Dns`/`IPEndPoint` 构造方式，而不是使用 `IPAddress.Parse`。
- 在 `_ReadStatusPayloadAsync` 中，循环被硬编码为最多只读取两个数据包；如果服务器发送额外的数据包（例如插件特有的数据包，或顺序不同），你可能希望放宽这个假设（例如循环直到同时拿到 status 和 pong，或直到超时 / EOF），而不是在遇到未知包类型时立刻抛异常。
## 逐条评论
### 评论 1
<location path="PCL.Core/Link/McPing/McPingService.cs" line_range="202-211" />
<code_context>
+ private async Task<(byte[] StatusPayload, long Latency)> _ReadStatusPayloadAsync(Stream stream, CancellationToken cancellationToken)
</code_context>
<issue_to_address>
**issue:** 当缺少 pong 时返回 0 ms 延迟会产生误导；建议暴露一个独立的状态。
在 `statusPayload` 非空但 `latency` 为 null 的路径中，当前方法返回 `(statusPayload, 0)`，将缺失/无效的 pong 与真实的 0 ms 延迟混为一谈。为避免这种情况，可以考虑在 `latency` 为 null 时抛出异常，将返回值签名改为可空延迟（例如 `long?`），或使用一个有文档说明的哨兵值（例如 `-1`）。
</issue_to_address>
### 评论 2
<location path="PCL.Core/Link/McPing/McPingService.cs" line_range="266-267" />
<code_context>
+ return BitConverter.ToInt64(buffer, 0);
+ }
+
+ private static async Task<byte[]> _ReadExactAsync(Stream stream, int length, CancellationToken cancellationToken)
+ {
+ var buffer = new byte[length];
+ var offset = 0;
+ while (offset < length)
+ {
+ var readLength = await stream.ReadAsync(buffer, offset, length - offset, cancellationToken);
+ if (readLength == 0)
+ throw new EndOfStreamException();
+ offset += readLength;
+ }
</code_context>
<issue_to_address>
**suggestion:** 提前结束流读取时抛异常，建议附带更多上下文信息，以方便调试。
目前 `EndOfStreamException` 在抛出时没有消息，也没有关于读取内容的任何细节。为提高生产环境诊断能力，建议附加一条描述性消息，并在可能的情况下包含期望读取的字节数与实际读取的字节数，以便明确是哪个读操作失败以及差了多少字节。
```suggestion
if (readLength == 0)
throw new EndOfStreamException(
$"Unexpected end of stream while attempting to read {length} bytes; only {offset} bytes were read.");
```
</issue_to_address>
### 评论 3
<location path="PCL.Core/Minecraft/ServerAddressResolver.cs" line_range="20-24" />
<code_context>
public static class ServerAddressResolver
{
+ public readonly record struct ResolvedServerAddress(string Host, string Ip, int Port);
+
// Minecraft Java 默认端口
</code_context>
<issue_to_address>
**issue (bug_risk):** 在某些回退路径中，`Ip` 仍然可能是主机名，这与它的语义以及后续使用方式相冲突。
在最终回退分支中，当 `_ResolveFirstIpAsync` 失败时，`Ip` 可能会被设置为 `idnHost`，但下游现在会调用 `IPAddress.Parse(addr.Ip)`，而这在遇到非字面量地址时会抛异常。请确保要么 `Ip` 始终是字面量 IP（并对需要主机名的调用方作相应调整），要么明确区分并维护独立的 `Host` 与 `Ip` 字段，并在回退路径中避免将主机名赋给 `Ip`。
</issue_to_address>
### 评论 4
<location path="PCL.Core/Link/McPing/McPingServiceFactory.cs" line_range="34-36" />
<code_context>
return new McPingService(ip, port, timeout);
}
+ public static IMcPingService CreateService(string host, IPEndPoint endpoint, int timeout = 10000)
+ {
+ return new McPingService(host, endpoint, timeout);
+ }
+
</code_context>
<issue_to_address>
**suggestion:** 避免将超时时间硬编码；请复用共享默认值以保持行为一致。
该重载将 `timeout = 10000` 写死，而不是使用现有的共享默认值（例如 `McPingService.DefaultTimeout`）。如果默认值将来发生变化，这个方法的行为会与其他地方不一致。更好的做法是：要么要求调用方显式传入 timeout，要么委托给使用共享默认值的构造函数，以保持行为一致。
建议实现如下：
```csharp
public static IMcPingService CreateService(string host, IPEndPoint endpoint)
{
return new McPingService(host, endpoint, McPingService.DefaultTimeout);
}
public static IMcPingService CreateService(string host, IPEndPoint endpoint, int timeout)
{
return new McPingService(host, endpoint, timeout);
}
```
这里假定 `McPingService.DefaultTimeout` 已经存在，并且是代码库中其他位置使用的共享默认值。如果该常量或属性的名称或位置不同，请相应调整引用（例如使用 `McPingOptions.DefaultTimeout` 或类似的共享配置值）。
</issue_to_address>帮我变得更有用！请在每条评论上点击 👍 或 👎，我会根据你的反馈改进后续审查。
Original comment in English
Hey - I've found 4 issues, and left some high level feedback:
- In
MinecraftServer.xaml.vbandPageInstanceServer.xaml.vb,IPAddress.Parse(addr.Ip)will throw ifIpcontains a hostname (which can happen in someResolvedServerAddressfallbacks); consider either ensuringIpis always an actual IP, or usingDns/IPEndPointconstruction that accepts hostnames instead ofIPAddress.Parse. - In
_ReadStatusPayloadAsync, the loop is hard-coded to read at most two packets; if a server sends additional packets (e.g., plugin-specific or in a different order), you may want to relax this assumption (e.g., loop until both status and pong are found or a timeout/EOF occurs) instead of immediately throwing on an unknown packet type.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `MinecraftServer.xaml.vb` and `PageInstanceServer.xaml.vb`, `IPAddress.Parse(addr.Ip)` will throw if `Ip` contains a hostname (which can happen in some `ResolvedServerAddress` fallbacks); consider either ensuring `Ip` is always an actual IP, or using `Dns`/`IPEndPoint` construction that accepts hostnames instead of `IPAddress.Parse`.
- In `_ReadStatusPayloadAsync`, the loop is hard-coded to read at most two packets; if a server sends additional packets (e.g., plugin-specific or in a different order), you may want to relax this assumption (e.g., loop until both status and pong are found or a timeout/EOF occurs) instead of immediately throwing on an unknown packet type.
## Individual Comments
### Comment 1
<location path="PCL.Core/Link/McPing/McPingService.cs" line_range="202-211" />
<code_context>
+ private async Task<(byte[] StatusPayload, long Latency)> _ReadStatusPayloadAsync(Stream stream, CancellationToken cancellationToken)
</code_context>
<issue_to_address>
**issue:** Returning 0 ms latency when pong is missing can be misleading; consider surfacing a distinct state.
In the path where `statusPayload` is non-null but `latency` is null, the method currently returns `(statusPayload, 0)`, conflating a missing/invalid pong with a real 0 ms latency. To avoid this, consider either throwing when `latency` is null, changing the signature to return a nullable latency (e.g. `long?`), or using a documented sentinel value (such as `-1`).
</issue_to_address>
### Comment 2
<location path="PCL.Core/Link/McPing/McPingService.cs" line_range="266-267" />
<code_context>
+ return BitConverter.ToInt64(buffer, 0);
+ }
+
+ private static async Task<byte[]> _ReadExactAsync(Stream stream, int length, CancellationToken cancellationToken)
+ {
+ var buffer = new byte[length];
+ var offset = 0;
+ while (offset < length)
+ {
+ var readLength = await stream.ReadAsync(buffer, offset, length - offset, cancellationToken);
+ if (readLength == 0)
+ throw new EndOfStreamException();
+ offset += readLength;
+ }
</code_context>
<issue_to_address>
**suggestion:** Include more context when throwing on premature stream end to ease debugging.
Currently the `EndOfStreamException` is thrown without a message or any details about what was being read. To improve production diagnostics, include a descriptive message and, if possible, the expected vs. actual bytes read so it’s clear which read operation failed and by how much.
```suggestion
if (readLength == 0)
throw new EndOfStreamException(
$"Unexpected end of stream while attempting to read {length} bytes; only {offset} bytes were read.");
```
</issue_to_address>
### Comment 3
<location path="PCL.Core/Minecraft/ServerAddressResolver.cs" line_range="20-24" />
<code_context>
public static class ServerAddressResolver
{
+ public readonly record struct ResolvedServerAddress(string Host, string Ip, int Port);
+
// Minecraft Java 默认端口
</code_context>
<issue_to_address>
**issue (bug_risk):** In some fallback paths `Ip` may still be a hostname, which conflicts with its semantics and downstream usage.
In the final fallback branch, `Ip` can be set to `idnHost` when `_ResolveFirstIpAsync` fails, but downstream now calls `IPAddress.Parse(addr.Ip)`, which will throw on non-literal addresses. Please either ensure `Ip` is always a literal IP (and adjust callers that need a hostname), or introduce/maintain a separate `Host` vs `Ip` distinction and avoid assigning a hostname to `Ip` in the fallback path.
</issue_to_address>
### Comment 4
<location path="PCL.Core/Link/McPing/McPingServiceFactory.cs" line_range="34-36" />
<code_context>
return new McPingService(ip, port, timeout);
}
+ public static IMcPingService CreateService(string host, IPEndPoint endpoint, int timeout = 10000)
+ {
+ return new McPingService(host, endpoint, timeout);
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Avoid hardcoding the timeout value; reuse the shared default to keep behavior consistent.
This overload hardcodes `timeout = 10000` instead of using the existing shared default (e.g. `McPingService.DefaultTimeout`). If the default changes, this method will behave differently. Prefer either requiring callers to pass a timeout explicitly, or delegating to the constructor that uses the shared default so behavior stays aligned.
Suggested implementation:
```csharp
public static IMcPingService CreateService(string host, IPEndPoint endpoint)
{
return new McPingService(host, endpoint, McPingService.DefaultTimeout);
}
public static IMcPingService CreateService(string host, IPEndPoint endpoint, int timeout)
{
return new McPingService(host, endpoint, timeout);
}
```
These edits assume that `McPingService.DefaultTimeout` exists and is the shared default used elsewhere in the codebase. If the constant or property has a different name or location, adjust the reference accordingly (e.g., `McPingOptions.DefaultTimeout` or a similar shared configuration value).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - 我发现了 3 个问题，并留下了一些高层次的反馈：
- 在
_ReadStatusPayloadAsync中，考虑给处理的数据包数量或总字节数增加一个硬性上限，这样即使服务器异常、持续发送非预期数据包，也不能一直让循环运行到外部超时才结束。 - 在
McPingServiceFactory.CreateService(string host, string? ip, int port = 25565)中，硬编码的10000超时时间与McPingService.DefaultTimeout是重复的；使用共享常量可以避免“魔法数字”，并保持各处超时时间一致。 - 在
LegacyMcPingService.PingAsync中，你创建了一个链接的CancellationTokenSource，并在其 token 上注册了回调，但没有释放该注册；将注册包在using中，或显式对其进行释放，可以防止在长连接场景下可能出现的处理程序泄漏。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_ReadStatusPayloadAsync`, consider adding a hard upper bound on the number of packets or total bytes processed so a misbehaving server that continuously sends unexpected packets can’t keep the loop running until the external timeout fires.
- In `McPingServiceFactory.CreateService(string host, string? ip, int port = 25565)`, the hardcoded `10000` timeout duplicates `McPingService.DefaultTimeout`; using the shared constant would avoid magic numbers and keep timeouts consistent.
- In `LegacyMcPingService.PingAsync`, you create a linked `CancellationTokenSource` and register a callback on its token but don’t dispose the registration; wrapping the registration in a `using` or explicitly disposing it would prevent potential handler leaks on long-lived sockets.
## Individual Comments
### Comment 1
<location path="PCL.Core/Link/McPing/McPingService.cs" line_range="90-97" />
<code_context>
var handshakePacket = _BuildHandshakePacket(_host, _endpoint.Port);
var statusPacket = _BuildStatusRequestPacket();
+ var pingTimestamp = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds();
+ var pingPacket = _BuildPingRequestPacket(pingTimestamp);
</code_context>
<issue_to_address>
**suggestion:** The ping timestamp is captured before handshake/status writes, which can inflate measured latency.
Because the timestamp is taken before sending handshake/status, any delay in writing or flushing those packets gets included in the RTT, overstating ping latency. Move the timestamp capture (and ping packet creation) to just before writing the ping so the measurement tracks only the ping round-trip.
Suggested implementation:
```csharp
var handshakePacket = _BuildHandshakePacket(_host, _endpoint.Port);
var statusPacket = _BuildStatusRequestPacket();
```
To fully implement the suggestion, you will also need to:
1. Locate the code where the ping request is sent (e.g., where `pingPacket` is written to the network stream or socket).
2. Just before writing the ping, add:
```csharp
var pingTimestamp = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds();
var pingPacket = _BuildPingRequestPacket(pingTimestamp);
```
3. Replace any usage of the old `pingTimestamp`/`pingPacket` (which used to be constructed near the handshake) with these new variables scoped near the send.
4. Ensure that the RTT calculation uses this `pingTimestamp` (captured right before sending) to avoid including handshake/status write delays in the ping latency.
</issue_to_address>
### Comment 2
<location path="PCL.Core/Link/McPing/McPingService.cs" line_range="193-200" />
<code_context>
+ private byte[] _BuildPingRequestPacket(long timestamp)
+ {
+ List<byte> pingRequest = [];
+ pingRequest.AddRange(VarIntHelper.Encode(9));
+ pingRequest.AddRange(VarIntHelper.Encode(1));
+ pingRequest.AddRange(BitConverter.GetBytes(timestamp).AsEnumerable().Reverse());
</code_context>
<issue_to_address>
**suggestion:** Avoid hardcoding the packet length `9` in the ping packet to reduce protocol coupling.
The literal `9` matches the current protocol (VarInt packet id 1 + 8‑byte long) but tightly couples the code to that specific layout. Instead, derive the length from the payload, e.g. `var payloadLength = 1 + sizeof(long);` then `VarIntHelper.Encode(payloadLength)`, so changes to the ping payload or id encoding won’t require updating a hardcoded value.
```suggestion
private byte[] _BuildPingRequestPacket(long timestamp)
{
// Packet ID is encoded as a VarInt with value 1, followed by an 8-byte long timestamp.
// Derive payload length from its components to avoid hardcoding protocol details.
const int packetIdLength = 1; // VarInt value 1 fits in a single byte
int payloadLength = packetIdLength + sizeof(long);
List<byte> pingRequest = [];
pingRequest.AddRange(VarIntHelper.Encode(payloadLength));
pingRequest.AddRange(VarIntHelper.Encode(1));
pingRequest.AddRange(BitConverter.GetBytes(timestamp).AsEnumerable().Reverse());
return pingRequest.ToArray();
}
```
</issue_to_address>
### Comment 3
<location path="PCL.Core/Link/McPing/McPingServiceFactory.cs" line_range="34-36" />
<code_context>
return new McPingService(ip, port, timeout);
}
+ public static IMcPingService CreateService(string host, string? ip, int port = 25565)
+ {
+ return CreateService(host, ip, port, 10000);
+ }
+
</code_context>
<issue_to_address>
**question (bug_risk):** The new overload defaults to a 10s timeout, which may be inconsistent with other factory methods.
This overload hardcodes `10000` as the timeout, whereas other factory methods rely on the `timeout` parameter or the service’s default. If `DefaultTimeout` (or current callers) use a different value, this introduces inconsistent behavior for callers using this overload. Please consider using the same default mechanism (e.g., a shared constant or existing default) so timeout behavior remains consistent across factory methods.
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进 Review 质量。
Original comment in English
Hey - I've found 3 issues, and left some high level feedback:
- In
_ReadStatusPayloadAsync, consider adding a hard upper bound on the number of packets or total bytes processed so a misbehaving server that continuously sends unexpected packets can’t keep the loop running until the external timeout fires. - In
McPingServiceFactory.CreateService(string host, string? ip, int port = 25565), the hardcoded10000timeout duplicatesMcPingService.DefaultTimeout; using the shared constant would avoid magic numbers and keep timeouts consistent. - In
LegacyMcPingService.PingAsync, you create a linkedCancellationTokenSourceand register a callback on its token but don’t dispose the registration; wrapping the registration in ausingor explicitly disposing it would prevent potential handler leaks on long-lived sockets.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_ReadStatusPayloadAsync`, consider adding a hard upper bound on the number of packets or total bytes processed so a misbehaving server that continuously sends unexpected packets can’t keep the loop running until the external timeout fires.
- In `McPingServiceFactory.CreateService(string host, string? ip, int port = 25565)`, the hardcoded `10000` timeout duplicates `McPingService.DefaultTimeout`; using the shared constant would avoid magic numbers and keep timeouts consistent.
- In `LegacyMcPingService.PingAsync`, you create a linked `CancellationTokenSource` and register a callback on its token but don’t dispose the registration; wrapping the registration in a `using` or explicitly disposing it would prevent potential handler leaks on long-lived sockets.
## Individual Comments
### Comment 1
<location path="PCL.Core/Link/McPing/McPingService.cs" line_range="90-97" />
<code_context>
var handshakePacket = _BuildHandshakePacket(_host, _endpoint.Port);
var statusPacket = _BuildStatusRequestPacket();
+ var pingTimestamp = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds();
+ var pingPacket = _BuildPingRequestPacket(pingTimestamp);
</code_context>
<issue_to_address>
**suggestion:** The ping timestamp is captured before handshake/status writes, which can inflate measured latency.
Because the timestamp is taken before sending handshake/status, any delay in writing or flushing those packets gets included in the RTT, overstating ping latency. Move the timestamp capture (and ping packet creation) to just before writing the ping so the measurement tracks only the ping round-trip.
Suggested implementation:
```csharp
var handshakePacket = _BuildHandshakePacket(_host, _endpoint.Port);
var statusPacket = _BuildStatusRequestPacket();
```
To fully implement the suggestion, you will also need to:
1. Locate the code where the ping request is sent (e.g., where `pingPacket` is written to the network stream or socket).
2. Just before writing the ping, add:
```csharp
var pingTimestamp = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds();
var pingPacket = _BuildPingRequestPacket(pingTimestamp);
```
3. Replace any usage of the old `pingTimestamp`/`pingPacket` (which used to be constructed near the handshake) with these new variables scoped near the send.
4. Ensure that the RTT calculation uses this `pingTimestamp` (captured right before sending) to avoid including handshake/status write delays in the ping latency.
</issue_to_address>
### Comment 2
<location path="PCL.Core/Link/McPing/McPingService.cs" line_range="193-200" />
<code_context>
+ private byte[] _BuildPingRequestPacket(long timestamp)
+ {
+ List<byte> pingRequest = [];
+ pingRequest.AddRange(VarIntHelper.Encode(9));
+ pingRequest.AddRange(VarIntHelper.Encode(1));
+ pingRequest.AddRange(BitConverter.GetBytes(timestamp).AsEnumerable().Reverse());
</code_context>
<issue_to_address>
**suggestion:** Avoid hardcoding the packet length `9` in the ping packet to reduce protocol coupling.
The literal `9` matches the current protocol (VarInt packet id 1 + 8‑byte long) but tightly couples the code to that specific layout. Instead, derive the length from the payload, e.g. `var payloadLength = 1 + sizeof(long);` then `VarIntHelper.Encode(payloadLength)`, so changes to the ping payload or id encoding won’t require updating a hardcoded value.
```suggestion
private byte[] _BuildPingRequestPacket(long timestamp)
{
// Packet ID is encoded as a VarInt with value 1, followed by an 8-byte long timestamp.
// Derive payload length from its components to avoid hardcoding protocol details.
const int packetIdLength = 1; // VarInt value 1 fits in a single byte
int payloadLength = packetIdLength + sizeof(long);
List<byte> pingRequest = [];
pingRequest.AddRange(VarIntHelper.Encode(payloadLength));
pingRequest.AddRange(VarIntHelper.Encode(1));
pingRequest.AddRange(BitConverter.GetBytes(timestamp).AsEnumerable().Reverse());
return pingRequest.ToArray();
}
```
</issue_to_address>
### Comment 3
<location path="PCL.Core/Link/McPing/McPingServiceFactory.cs" line_range="34-36" />
<code_context>
return new McPingService(ip, port, timeout);
}
+ public static IMcPingService CreateService(string host, string? ip, int port = 25565)
+ {
+ return CreateService(host, ip, port, 10000);
+ }
+
</code_context>
<issue_to_address>
**question (bug_risk):** The new overload defaults to a 10s timeout, which may be inconsistent with other factory methods.
This overload hardcodes `10000` as the timeout, whereas other factory methods rely on the `timeout` parameter or the service’s default. If `DefaultTimeout` (or current callers) use a different value, this introduces inconsistent behavior for callers using this overload. Please consider using the same default mechanism (e.g., a shared constant or existing default) so timeout behavior remains consistent across factory methods.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
tangge233
previously requested changes
Apr 11, 2026
Contributor
tangge233
left a comment
tangge233
left a comment
There was a problem hiding this comment.
协议的 JSON 处理没有问题，你真正修复的方式是额外传入 host 字段，在构造握手包的时候传入 host 而不是 ip。
下次请自己先 review AI 更改
ruattd
requested changes
Apr 11, 2026
Contributor
|
再次提醒，如果你不确保真的看懂了 AI 写了什么就不要浪费 reviewer 的时间了 |
ruattd
previously approved these changes
Apr 12, 2026
LinQingYuu
requested changes
Apr 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
修复：
额外更改：
Close #2518
Summary by Sourcery
通过正确封装状态响应并添加符合协议的 ping/延迟处理，修复了现代版 Minecraft 服务器 ping。
Bug Fixes（错误修复）:
Enhancements（增强改进）:
McPingService及其工厂方法，增加同时接受主机名和已解析端点的重载，以实现更精确的协议握手。Original summary in English
Summary by Sourcery
Fix modern Minecraft server ping by correctly framing status responses and adding protocol-compliant ping/latency handling.
Bug Fixes:
Enhancements:
Bug 修复：
增强内容：
McPingService工厂方法和构造函数重载，使其可以同时接受原始主机名和解析后的终结点，以实现更准确的协议握手。Original summary in English
Summary by Sourcery
通过正确封装状态响应并添加符合协议的 ping/延迟处理，修复了现代版 Minecraft 服务器 ping。
Bug Fixes（错误修复）:
Enhancements（增强改进）:
McPingService及其工厂方法，增加同时接受主机名和已解析端点的重载，以实现更精确的协议握手。Original summary in English
Summary by Sourcery
Fix modern Minecraft server ping by correctly framing status responses and adding protocol-compliant ping/latency handling.
Bug Fixes:
Enhancements: