Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Try to fix ISeeYou(#26) #242

Closed
wants to merge 1 commit into from
Closed

Try to fix ISeeYou(#26) #242

wants to merge 1 commit into from

Conversation

Lumine1909
Copy link
Contributor

@Lumine1909 Lumine1909 commented Jun 28, 2024 •
edited
Loading

原问题: MC-XiaoHei/ISeeYou#26

原因：异步线程处理pack时 主线程更改了传入的trackedValue 导致CME

这是我能想到的破坏性最小的解决方法 理论上不会有副作用

s-yh-china
s-yh-china reviewed Jun 28, 2024
View reviewed changes
patches/server/0105-Replay-Mod-API.patch Outdated
private static void pack(List<SynchedEntityData.DataValue<?>> trackedValues, RegistryFriendlyByteBuf buf) {
try (var ignored = io.papermc.paper.util.DataSanitizationUtil.start(true)) { // Paper - data sanitization
- for (SynchedEntityData.DataValue<?> dataValue : trackedValues) {
+ for (SynchedEntityData.DataValue<?> dataValue : new ArrayList<>(trackedValues)) { // Leaves - copy to avoid ConcurrentModificationException
Copy link
Member

Choose a reason for hiding this comment

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

或许我们可以只在replay里进行处理?
复制还是会对性能造成一些影响

Copy link
Member

Choose a reason for hiding this comment

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

还是得copy。你用copy on write array本质还是copy。

Copy link
Member

Choose a reason for hiding this comment

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

我的意思是 正常情况下不需要copy吧

Copy link
Member

Choose a reason for hiding this comment

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

现在的问题是 arraylist 不是线程安全的，在遍历的时候会直接报错。

@Lumine1909
Copy link
Contributor Author

那这个就按v姐的写法 用另一个safe codec处理？

@MC-XiaoHei
Copy link
Member

能不能延迟保存，就是等pack的引用计数到1再保存，这样就不会cme了

@Bluemangoo
Copy link
Member

你写写看？

@Lumine1909
Copy link
Contributor Author

能不能延迟保存，就是等pack的引用计数到1再保存，这样就不会cme了

不太可能 因为传入的data一直在track 除非删掉实体

@MC-XiaoHei
Copy link
Member

能不能延迟保存，就是等pack的引用计数到1再保存，这样就不会cme了

不太可能 因为传入的data一直在track 除非删掉实体

不可能啊，你netty收到pack后肯定不可能一直留着，解析完引用计数应该就归零了才对

@Lumine1909
Copy link
Contributor Author

能不能延迟保存，就是等pack的引用计数到1再保存，这样就不会cme了

不太可能 因为传入的data一直在track 除非删掉实体

不可能啊，你netty收到pack后肯定不可能一直留着，解析完引用计数应该就归零了才对

这里是发包不是收包 而且是在StreamCodec#encode时似的 传入的是一直引用的实体的数据

@MC-XiaoHei
Copy link
Member

能不能延迟保存，就是等pack的引用计数到1再保存，这样就不会cme了

不太可能 因为传入的data一直在track 除非删掉实体

不可能啊，你netty收到pack后肯定不可能一直留着，解析完引用计数应该就归零了才对

这里是发包不是收包 而且是在StreamCodec#encode时似的 传入的是一直引用的实体的数据

那发包时候是怎么处理的（
就是硬耗时吗
那看起来好像复制一份也不是不行，毕竟内存copy的消耗应该并不大

@Lumine1909
Copy link
Contributor Author

Lumine1909 commented Jul 4, 2024 •
edited
Loading

能不能延迟保存，就是等pack的引用计数到1再保存，这样就不会cme了

不太可能 因为传入的data一直在track 除非删掉实体

不可能啊，你netty收到pack后肯定不可能一直留着，解析完引用计数应该就归零了才对

这里是发包不是收包 而且是在StreamCodec#encode时似的 传入的是一直引用的实体的数据

那发包时候是怎么处理的（ 就是硬耗时吗 那看起来好像复制一份也不是不行，毕竟内存copy的消耗应该并不大

Leaves的Recorder继承了Connection，发包时会调用到send方法，然后保存数据包
当发送SetEntityData时会传入trackedDataValues，此时packet和实体共用同一个引用
leaves保存packet和实体tick不在一个线程
而保存过程调用了ClientboundSetEntityDataPacket#pack 一旦实体在这个过程中有数据修改就会报CME

ps:我甚至怀疑这个问题之前一直存在 只是这次才发现 因为配置文件没有影响savepacket时的行为

@MC-XiaoHei
Copy link
Member

那么问题来了，netty发数据总得变成bytebuf，能不能针对bytebuf操作呢

@MC-XiaoHei
Copy link
Member

#269
链接到这个吧，讲实话我更倾向于基于ByteBuf处理（

@MC-XiaoHei
Copy link
Member

@Lumine1909 你觉得基于ByteBuf好做吗

s-yh-china
s-yh-china approved these changes Jul 22, 2024
View reviewed changes
Copy link
Member

@s-yh-china s-yh-china left a comment

Choose a reason for hiding this comment

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

先合了吧

patches/server/0105-Replay-Mod-API.patch Outdated
private static void pack(List<SynchedEntityData.DataValue<?>> trackedValues, RegistryFriendlyByteBuf buf) {
try (var ignored = io.papermc.paper.util.DataSanitizationUtil.start(true)) { // Paper - data sanitization
- for (SynchedEntityData.DataValue<?> dataValue : trackedValues) {
+ for (SynchedEntityData.DataValue<?> dataValue : new ArrayList<>(trackedValues)) { // Leaves - copy to avoid ConcurrentModificationException
Copy link
Member

Choose a reason for hiding this comment

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

我的意思是 正常情况下不需要copy吧

@Bluemangoo
Copy link
Member

先同步。

@Lumine1909
Copy link
Contributor Author

这种实现太恶心了 各位有没有更好的想法qwq

@MC-XiaoHei
Copy link
Member

这种实现太恶心了 各位有没有更好的想法qwq

基于bytebuf？

Lumine1909
Lumine1909 commented Jul 25, 2024
View reviewed changes
patches/server/0097-Replay-Mod-API.patch
@@ -1077,6 +1116,8 @@ index 0000000000000000000000000000000000000000..331ced8677b28827c277c64ae0b31d4d
+import net.minecraft.network.protocol.Packet;
+import net.minecraft.network.protocol.PacketFlow;
+import net.minecraft.network.protocol.configuration.ConfigurationProtocols;
+import net.minecraft.network.protocol.game.ClientboundAddEntityPacket;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

这里后面会改qwq

Lumine1909
Lumine1909 commented Jul 25, 2024
View reviewed changes
patches/server/0097-Replay-Mod-API.patch
@@ -1155,8 +1197,15 @@ index 0000000000000000000000000000000000000000..331ced8677b28827c277c64ae0b31d4d
+ throw new IllegalArgumentException("Unknown protocol state " + state);
+ }
+
+ ByteBuf buf = Unpooled.buffer();
+ protocol.codec().encode(buf, packet);
+ // Extremely ugly implementation, needs discussion and optimization
Copy link
Contributor Author

Choose a reason for hiding this comment

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

自己写的自己看了都犯恶心

@Bluemangoo
Copy link
Member

我倾向于抄原版发包逻辑，但是在序列化的位置改成保存。

@MC-XiaoHei
Copy link
Member

我倾向于抄原版发包逻辑，但是在序列化的位置改成保存。

之前就是那样啊 问题是发包的那个包会引用数据，数据会实时更改

这就导致必须等待保存完，不然就CME

@MC-XiaoHei
Copy link
Member

MC-XiaoHei commented Jul 25, 2024 •
edited
Loading

基于bytebuf可能是唯一一个不需要复制额外内存 还能解决问题的方法（

至于实现好不好看 其实我觉得不是很重要 服务端里面塞点黑魔法怎么了（x

@Bluemangoo
Copy link
Member

我还是支持复制。

@MC-XiaoHei
Copy link
Member

我还是支持复制。

一定要复制吗（）虽然好像确实不多

@MC-XiaoHei
Copy link
Member

其实就是CPU换Mem还是Mem换CPU

@MC-XiaoHei
Copy link
Member

鉴于生电服一贯的CPU性能不足 似乎确实应该复制

@Bluemangoo
Copy link
Member

磁盘io不是更吃性能？你还缺这点？

@MC-XiaoHei
Copy link
Member

那就复制吧（

@MC-XiaoHei
Copy link
Member

MC-XiaoHei commented Jul 25, 2024 •
edited
Loading

这样要不直接写Recorder API吧

支持记录要发给玩家的包

然后基于Recorder写Photographer API，就是只需要写假人屏蔽相关

@Bluemangoo
Copy link
Member

我支持。

@Lumine1909
Copy link
Contributor Author

Lumine1909 commented Jul 25, 2024 •
edited
Loading

这样要不直接写Recorder API吧

支持记录要发给玩家的包

然后基于Recorder写Photographer API，就是只需要写假人屏蔽相关

有道理 与其修修补补不如推倒重来
天下苦历史包袱久也（乱叫

而且很方便解决 可以用上现有的Bytebuf api

@MC-XiaoHei
Copy link
Member

assigned to Lumine(x

话说为什么要用ByteBufAPI

@Bluemangoo
Copy link
Member

神他妈assign pr。

@MC-XiaoHei
Copy link
Member

神他妈assign pr。

草 我应该在issue assign（

@s-yh-china
Copy link
Member

那这个就先关了

@s-yh-china s-yh-china closed this Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s-yh-china s-yh-china s-yh-china approved these changes

@Bluemangoo Bluemangoo Bluemangoo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Lumine1909 @MC-XiaoHei @Bluemangoo @s-yh-china