Process SkinData asynchronously

[Blackjack200]

Summary

PocketMine-MP/src/entity/Skin.php

Lines 69 to 74 in fbaa125

	if($geometryData !== ""){
	try{
	$decodedGeometry = (new CommentedJsonDecoder())->decode($geometryData);
	}catch(\RuntimeException $e){
	throw new InvalidSkinException("Invalid geometry data: " . $e->getMessage(), 0, $e);
	}

This pull request optimizes the processing of player geometry data by offloading skin decoding to a ProcessSkinTask. This change addresses potential server freezes caused by the decoding of overly complex JSON structures.

Existing Problems

Decoding geometry data directly in the main thread was a time-intensive operation, especially when the JSON data was highly complex, leading to server performance issues or even temporary freezes.

• Before patch: Timings report
• After patch: Timings report

Changes

• Offloaded the skin geometry decoding to an asynchronous task (ProcessSkinTask) to prevent the main thread from being froze.

Behavioural Changes

• Players' skin geometry data will now be processed asynchronously.
• Reduced risk of server performance degradation during skin decoding.

Backwards Compatibility

This change is fully backward compatible, as it does not modify the API or the format of geometry data. Existing player skins and data handling will continue to work without modification.

Follow-up

• Monitor server performance post-deployment to ensure stability.

Tests

• Verified that skin geometry data is correctly decoded when processed via ProcessSkinTask.
• Tested gameplay to confirm no adverse effects on player skins or entity rendering.
• Compared server performance before and after the patch, as reflected in the timings reports.

[dktapps]

This is probably a good idea. Only other way we could bring down this cost is if there was some faster way to strip and validate the JSON, which as far as I'm aware there isn't.

However, there's some potential issues that need to be addressed.

[dktapps]

@pmmp/server-developers anyone else want to cast an eye over this?

[ipad54]

This would probably make the server to freeze / crash if a malicious client floods tons of skin change requests per tick.

[Blackjack200]

Would the existing $this->gamePacketLimiter be sufficient for skin change packets, or would it be better to implement a separate rate limiter specifically for PlayerSkinPacket to prevent potential flooding?

[Blackjack200]

If a separate rate limiter is needed, how should we set the values for INCOMING_SKIN_PACKET_PER_TICK = 1 and INCOMING_SKIN_PACKET_BUFFER_TICKS = 100?
Would these settings be appropriate, or should we adjust them further?

[dktapps]

Would the existing $this->gamePacketLimiter be sufficient for skin change packets, or would it be better to implement a separate rate limiter specifically for PlayerSkinPacket to prevent potential flooding?

Probably not. It's supposed to limit all packets, so it has quite generous limits.

More so the issue here is memory overload if the client spams a lot of skin change packets (it could cause a large backlog in the worker pool & hog a lot of memory).

My thought was to limit the number of concurrent ProcessSkinTasks per session, but I'm not sure if this is the best way.

[Blackjack200]

Limiting the rate of PlayerSkinPacket packets seems easier to implement and would have a similar effect to limiting the number of concurrent ProcessSkinTask.

In my preliminary testing, setting INCOMING_SKIN_PACKET_PER_TICK = 0.1 and INCOMING_SKIN_PACKET_BUFFER_TICKS = 20 seems promising (though it still needs sufficient testing). However, this would require changing the $averagePerTick in PacketLimiter from an int to a float or both. Would that be acceptable?

[dktapps]

No, I don't think so. The number of packets isn't the only factor we need to consider. We also have to be aware of how long each packet takes to process.

Under normal conditions the packets might take 10 ms each to process, but an attacker could craft packets that take 10-100x longer to process and still be subject to the same packets-per-tick limits (getting more lag per packet). That's why I said we probably need to limit the number of tasks, rather than the number of packets.

[Blackjack200]

No, I don't think so. The number of packets isn't the only factor we need to consider. We also have to be aware of how long each packet takes to process.

Under normal conditions the packets might take 10 ms each to process, but an attacker could craft packets that take 10-100x longer to process and still be subject to the same packets-per-tick limits (getting more lag per packet). That's why I said we probably need to limit the number of tasks, rather than the number of packets.

Aside from the better methods for now, we could consider handling these potential time-bomb data by spawning new processes, similar to ConsoleReaderChildProcess. This would allow the main thread to control their runtime, using OS-level process killing (with proper abstraction for generalization—I’ve already implemented this in my private project, leveraging opis/closure). However, this approach does come with challenging compatibility issues.

[dktapps]

That sounds overengineered to me.

[dktapps]

#4981 would also enable skipping some validation, e.g. if we had a map<rawData, validatedOptimizedData>
However this would only be helpful for dealing with common geometries

[dktapps]

This PR would be better if we could capture PlayerSkinPacket immediately after DataPacketDecodeEvent and have it handed off to an async task in its plain buffer form.

This would:

• eliminate main thread skin decoding performance cost entirely
• avoid unnecessary serialize cost of SkinData (since it would never exist on the main thread)

However, it would break plugins that use DataPacketReceiveEvent to capture the packet.

On an unrelated note, worth mentioning (this only just occurred to me this second) is that this change would be disruptive for any plugins which set custom SkinAdapters in TypeConverter, as these won't be configured on the other threads.