Implement waiting login sequence

[ShockedPlot7560]

Introduction

This PR introduces a new system for deferred the player creation, allowing plugins to add tasks to be done at that time. This ensures that plugins load player data correctly before the player is created and the login sequence launched.

Plugins can also cancel the player's connection by rejecting their promise.

Changes

API changes

• Add Promise::all()

Behavioural changes

• Player creation sequence is launched only when all promises added by plugins are resolved.

Tests

class Main extends PluginBase implements Listener{
	protected function onEnable() : void{
		$this->getServer()->getPluginManager()->registerEvents($this, $this);
	}

	public function onPlayerCreated(PlayerPreCreationEvent $event) : void {
		$resolver = new PromiseResolver();
		$this->getLogger()->info("Player " . $event->getNetworkSession()->getDisplayName() . " pre creation");
		$event->addPromise($resolver->getPromise());
		$this->getScheduler()->scheduleDelayedTask(new ClosureTask(function() use ($event, $resolver) {
			$this->getLogger()->info("Player " . $event->getNetworkSession()->getDisplayName() . " pre creation (delayed)");
			$resolver->resolve(null); // Connect the player
			// OR
			$resolver->reject(); // Disconnect the player
		}), 5 * 20);
	}
}

With promise resolve:

[16:21:18.114] [Server thread/DEBUG]: [NetworkSession: ShockedPlot7560] Resource packs sequence completed
[16:21:18.609] [Server thread/INFO]: [TestPlugin] Player ShockedPlot7560 pre creation
[16:21:23.071] [Server thread/INFO]: [TestPlugin] Player ShockedPlot7560 pre creation (delayed)

With promise rejection:

[16:46:59.301] [Server thread/INFO]: [TestPlugin] Player ShockedPlot7560 pre creation
[16:47:04.276] [Server thread/INFO]: [TestPlugin] Player ShockedPlot7560 pre creation (delayed)
[16:47:04.276] [Server thread/INFO]: [NetworkSession: ShockedPlot7560] Session fermée : Failed to create player (ID d'erreur: cfb2-7e1a-f645)
[16:47:04.276] [Server thread/ERROR]: [NetworkSession: ShockedPlot7560] Failed to create player

[dktapps]

I'd be happier if there was a way to do this without adding new events. I don't really see an obvious reason why a new event is needed here.

[ShockedPlot7560]

I'd be happier if there was a way to do this without adding new events. I don't really see an obvious reason why a new event is needed here.

This can be done via callbacks, in the same way as inventory listener or craftmanager.
Is it better? I'm currently hesitating between the event and this solution.

[dktapps]

Well, I don't see why e.g. PlayerLoginEvent couldn't be used for this. Perhaps even PlayerCreationEvent itself.

[ShockedPlot7560]

The idea was that this would allow things to be executed before the event login. But the more I think about it, the more I realize that it could be done in the PlayerCreationEvent or during the login event.

[ShockedPlot7560]

I moove the logic to PlayerCreationEvent. It's not really enviable to place it in the PlayerLoginEvent, in the sense that my ambition is that we can specify tasks to be executed before the event is launched. These tasks are executed before the login sequence is launched: spawn, log console and change handler for player spawn.

The event's documentation needs to be reviewed, because it is no longer dedicated to changing the player's class, but also contains access to the WaitGroup

[dktapps]

I'm getting the impression that this "wait group" is essentially just a Promise combined with a semaphore, except that this has no reject() functionality.

The documentation is perhaps too much. People are going to get scared off by seeing that much text.

[ShockedPlot7560]

I've completely rewritten the system by simply adding Promise::all

From now on, plugins will be able to add promises that must be completed before the player is created.

[ShockedPlot7560]

Now requires a new translation: the message that will be displayed when the player disconnects because the promise has been rejected.

[dktapps]

Well, after some testing, it's more interesting to move this functionality before launching PlayerPreLogin. I was able to carry out player disconnection tests at the time of PlayerCreation, PlayerPreLogin and PlayerLogin, and only the prelogin outputted the error message 100% correctly. In the other cases, a "disconnected from server" appears.

It wouldn't be a good idea to do it at login time either, as this would mean processing the LoginPacket when the player isn't necessarily authorized to connect (in the case of a plugin checking a player's ban in the database, for example).

Whether or not the client functions correctly shouldn't be relevant. Also, doing this before PlayerPreLogin could be problematic for loading playerdata from remote databases, which is the primary use case for a feature like this.

[ShockedPlot7560]

Would you suggest putting it between the prelogin and the login?

[dktapps]

I'm undecided, but I don't think prelogin is the right call.

[dries-c]

Ideally this should be ran after PlayerLoginEvent as to not waste resources on gathering data for players that will never connect or to not overload the database whenever people are spamming fake accounts.

[ShockedPlot7560]

I would also agree

[dktapps]

I'm leaning towards doing it directly after PlayerCreationEvent, to permit the player to be properly constructed with the appropriate data. It would be nice if Player::__construct() could receive all of its data and then be able to proceed directly to the spawn sequence IMO.

[ShockedPlot7560]

I'm leaning towards doing it directly after PlayerCreationEvent, to permit the player to be properly constructed with the appropriate data. It would be nice if Player::__construct() could receive all of its data and then be able to proceed directly to the spawn sequence IMO.

I don't think that passing values to the constructor is relevant for this PR, but I wouldn't agree with that too much.
We'd like to do away with custom players and allow plugins to inject values directly into the player. Perhaps this is another vision you had in mind?

[dktapps]

I'm thinking of stuff like the ?CompoundTag containing the player's data. But yes, you're probably right, since custom data would be used to configure plugin sessions, rather than the player itself.

[ShockedPlot7560]

I think it would be better to rename the event into PlayerPreCreationEvent event because it will be called just before player creation (accorded by @dktapps' comment) and it will be called just after the PlayerCreationEvent (destined to disappear).

Afterwards, I also agree with @dries-c's opinion in the sense that applying it after login would avoid doing things on a potentially invalid player, a bot. Potentially to be done in another PR to allow some flexibility to plugins and thus have the choice? There may be cases where I see the need for these two events.

[dktapps]

because it will be called just before player creation (accorded by @dktapps' comment)

This was a suggestion, not a request. Your point about removing custom player classes changed my mind.

Afterwards, I also agree with @dries-c's opinion in the sense that applying it after login would avoid doing things on a potentially invalid player, a bot. Potentially to be done in another PR to allow some flexibility to plugins and thus have the choice? There may be cases where I see the need for these two events.

I don't see a need for two events. Invalid players won't get any further than PlayerPreLoginEvent anyway.

[ShockedPlot7560]

This was a suggestion, not a request. Your point about removing custom player classes changed my mind.

I know, but I thought the comment was pertinent and would perhaps allow more flexibility in the future to have this event fired before the player is built and not after.

[ShockedPlot7560]

We now use the PlayerPreCreationEvent, which is launched when the player is created. Once the promises have been completed, the PlayerCreationEvent is launched.

[ShockedPlot7560]

What about merging this ? Is there need some modification or is it clear for the moment ?

[ShockedPlot7560]

I think we need a general way of making events asynchronous before we change the logic of these events. Closing