Improvement for Player->forms

[leolee3914]

Related issues & PRs

Changes

• player have a maximum of 30 pending forms. it should be enough for any situation. oldest forms will be removed when the number of forms exceeds the limit. this is to prevent the client requesting the form continuously but not submitting it
• remove all pending forms on calling Player->closeAllForms()
• unset Player->forms on Player->destroyCycles() because class extending \pocketmine\form\Form in plugin may contain Player class variable which may lead to memory leak

API changes

Behavioural changes

Backwards compatibility

Follow-up

Tests

i haven't tested yet

[dktapps]

There are too many unrelated changes in this PR. Cycle destroying makes sense, but I'm not a fan of imposing an arbitrary hard limit on forms, particularly since it won't stop the player spamming forms anyway.

[dktapps]

This could just set the field to [] instead of unsetting

[dktapps]

This really ought to trigger onClose for the affected forms

[leolee3914]

there is no onClose() method

[dktapps]

Give null to the response handler, that would be what the client would do

[leolee3914]

on my plugin, the handler may send the same form to player again when the response is null. this is not expected and may cause another issue
also the client can ignore the forms sent by server like this

[dktapps]

Well, the forms should know that they've been discarded in my opinion. As far as I know, the current method will cause the client to send close responses for any sent forms anyway, so this would just be mirroring the existing behaviour. (Correct me if I'm wrong.)

[leolee3914]

PMMP should not trust the client behavior
the solution for me is just ignoring it or adding Form->onDrop() (BC break)

[leolee3914]

for some server i played, player pressing "X" (= passing null to handler) to close the form will back to previous page (= send new form)

[dktapps]

I don't like this. This seems very arbitrary, and it also doesn't notify the sender that the form was discarded

[leolee3914]

for limit of pending forms, try this and check the memory used

class Dummy extends PluginBase implements Listener {

	function onEnable () : void {
		$this->getServer()->getPluginManager()->registerEvents($this, $this);
	}

	function cmd ( CommandEvent $e ) : void {
		$p = $e->getSender();
		if ( $e->getCommand() === 'openform' and $p instanceof Player ) {
			$p->sendForm(new class() implements \pocketmine\form\Form {
				var $dummy;
				public function jsonSerialize () {
					$this->dummy = random_bytes(10000);
					return '{}';
				}
				public function handleResponse (Player $player, $data) : void {
					exit;
				}
			});
			(function () {
				/** @var Player $this */
				
				var_dump(count($this->forms));
			})->call($p);
		}
	}

}

fake player by https://github.com/PrismarineJS/bedrock-protocol
npm install bedrock-protocol
node xxx.js

const bedrock = require('bedrock-protocol')
const client = bedrock.createClient({
  host: 'localhost',   // optional
  port: 19133,         // optional, default 19132
  username: 'L3S9',   // the username you want to join as, optional if online mode
  offline: true       // optional, default false. if true, do not login with Xbox Live. You will not be asked to sign-in if set to true.
})

client.on('play_status', function (pk) {
	if ( pk.status === 'player_spawn' ) {
		setInterval(function () {
			client.queue('text', {
				needs_translation: false,
				category: 'authored',
				type: 'chat',
				xuid: '',
				platform_chat_id: '',
				has_filtered_message: false,
				message: '/openform',
				source_name: '',
			});
		}, 25);
	}
});

[leolee3914]

this is the result on my PC

int(47288)
Command output | ---- Server status ----
Command output | Uptime: 26 minutes 48 seconds
Command output | Current TPS: 20 (1.96%)
Command output | Average TPS: 20 (1.75%)
Command output | Network upload: 4.45 kB/s
Command output | Network download: 1.96 kB/s
Command output | Thread count: 21
Command output | Main thread memory: 666.79 MB.
Command output | Total memory: 666.79 MB.
Command output | Total virtual memory: 682.00 MB.
Command output | World "world": 344 loaded chunks, 36 ticking chunks, 1 entities. Time 0.59ms
int(47289)
int(47290)
int(47291)
.
.
.
Command output | ---- Server status ----
Command output | Uptime: 37 minutes 15 seconds
Command output | Current TPS: 20 (2.15%)
Command output | Average TPS: 20 (2.22%)
Command output | Network upload: 3.32 kB/s
Command output | Network download: 1.46 kB/s
Command output | Thread count: 3
Command output | Main thread memory: 914.32 MB.
Command output | Total memory: 914.32 MB.
Command output | Total virtual memory: 928.00 MB.
Command output | World "world": 344 loaded chunks, 36 ticking chunks, 1 entities. Time 0.47ms
int(67481)
int(67482)
int(67483)
int(67484)

[dktapps]

You should be rate limiting this yourself rather than making PM impose general limits. PM isn't responsible for the means by which forms are sent.

If you don't limit it yourself, the player will get lots of copies of the same form anyway, which isn't good in any case.

[leolee3914]

i think PMMP has responsibility to limit it. this is count limit of array instead of rate limit. it is easy to waste server memory but this will take some time
there is no reminder to tell plugin devs to add any limit and Player->forms is not accessible for plugin to remove the old forms