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.

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

Generalize asynchronous events #6092

Open
wants to merge 23 commits into
base: minor-next
Choose a base branch
from
Open

Conversation

ShockedPlot7560
Copy link
Member

@ShockedPlot7560 ShockedPlot7560 commented Oct 14, 2023

Introduction

This PR would be more of a foretaste for #6015 in that it provides a means of generalizing asynchronous event calling.

An asynchronous event in this context is one whose action can be deferred by plugins specifying promises to be resolved.
The integrity of event priorities is guaranteed by calling them one by one. Before moving on to the next priority, we wait until all the promises of the current one have been fulfilled.
Only the state of the server and player context can't be guaranteed. However, the state of the event is unaffected, whether it is called asynchronous or synchronous.

Behaviour

When a listener of an asynchronous event returns a Promise, the event is marked as asynchronous and placed at the end of the call stack for its priority.
When a callAsync is called, the call flow is as follows:

1. for each priority:
   2. call async handlers which allows concurrent execution
   3. for each async handlers which doesn't allows concurrent execution:
       1. wait for previous promises
       2. clear all the promise
       3. call the handler and add the return promise into the waiting list
   4. wait for all promises
2. the promise returned by callAsync is resolved with their instance

In this way, each listener can add only one promise.
If, on the other hand, the handler is sensitive to changes in the event, it is preferable to add the @exclusiveCall tag, which specifies that it must be the only one being called.

Event transformation

If we wanted to implement the asynchronous chat event, this would look like this:

class PlayerChatAsyncEvent extends AsyncEvent implements Cancellable{
	use CancellableTrait;
... Logic here
}

When calling:

$ev = new PlayerChatAsyncEvent($this, $messagePart, $this->server->getBroadcastChannelSubscribers(Server::BROADCAST_CHANNEL_USERS), new StandardChatFormatter());
$ev->callAsync()->onCompletion(function(PlayerChatAsyncEvent $ev) : void{
	if(!$ev->isCancelled()){
		$this->server->broadcastMessage($ev->getFormatter()->format($ev->getPlayer()->getDisplayName(), $ev->getMessage()), $ev->getRecipients());
	}
}, function() {
	// NOOP
});

Changes

API changes

  • Add AsyncEvent class
  • Add Promise::all()

Behavioural changes

Backwards compatibility

Not for the moment

Follow-up

n/a

Tests

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

	public function onPlayerChatAsyncConc1(PlayerChatEvent $event) : Promise {
		return $this->timeout("Player " . $event->getPlayer()->getName() . " chat async concurrent 1");
	}

	public function onPlayerChatAsyncConc2(PlayerChatEvent $event) : Promise {
		return $this->timeout("Player " . $event->getPlayer()->getName() . " chat async concurrent 2");
	}

	/**
	 * @exclusiveCall
	 */
	public function onPlayerChatAsyncNoConc(PlayerChatEvent $event) : Promise {
		return $this->timeout("Player " . $event->getPlayer()->getName() . " chat async exclusiveCall");
	}

	private function timeout(string $message) : Promise {
		$promise = new PromiseResolver();

		$this->getScheduler()->scheduleDelayedTask(new ClosureTask(function() use ($message, $promise) {
			$this->getLogger()->debug("Resolving promise for message " . $message);
			$promise->resolve(null);
		}), 5 * 20);

		return $promise->getPromise();
	}
}

code came from #6015 and will be subject to probable changes
An asynchronous event is one that allows the addition of promises to be resolved before being completed.
This implementation integrates priority levels, allowing you to wait for all promises added to one level before moving on to the next.
This is made possible by separating the event call logic into several functions, which can then be integrated into AsyncEventTrait::callAsync()
@ShockedPlot7560 ShockedPlot7560 added Category: API Related to the plugin API Category: Core Related to internal functionality Type: Enhancement Contributes features or other improvements to PocketMine-MP labels Oct 14, 2023
@ShockedPlot7560 ShockedPlot7560 added this to Async events in Events API Oct 14, 2023
@SOF3 SOF3 self-requested a review October 15, 2023 02:36
src/promise/Promise.php Outdated Show resolved Hide resolved
src/promise/Promise.php Outdated Show resolved Hide resolved
src/promise/Promise.php Outdated Show resolved Hide resolved
src/promise/Promise.php Outdated Show resolved Hide resolved
src/event/AsyncEventTrait.php Outdated Show resolved Hide resolved
src/event/AsyncEventTrait.php Outdated Show resolved Hide resolved
src/event/AsyncEventTrait.php Outdated Show resolved Hide resolved
src/event/Event.php Outdated Show resolved Hide resolved
If the event is async and the handler return Promise, it will be handle as an async event.
However, if the event is async and the handler return smth different than Promise, it will be handle synchronously.
An async handler can specify if it wishes to be called with no concurrent handlers with the tag @noConcurrentCall
@ShockedPlot7560
Copy link
Member Author

New behaviour

Above all, the logic of synchronous events remains the same, even for asynchronous events, as long as they are called synchronously.

When a listener of an asynchronous event returns a Promise, the event is marked as asynchronous and placed at the end of the call stack for its priority.
When a callAsync is called, the call flow is as follows:

1. for each priority:
   1. call sync handlers (return type is different than Promise).
   2. call async handlers which allows concurrent execution
   3. for each async handlers which doesn't allows concurrent execution:
       1. wait for previous promises
       2. clear all the promise
       3. call the handler and add the return promise into the waiting list
   4. wait for all promises
2. the promise returned by callAsync is resolved

In this way, each listener can add only one promise.
If, on the other hand, the handler is sensitive to changes in the event, it is preferable to add the @noConcurrentCall tag, which specifies that it must be the only one being called.

@ShockedPlot7560 ShockedPlot7560 changed the title Add "async" events Generalize asynchronous events Oct 22, 2023
Copy link
Member

@SOF3 SOF3 left a comment

Choose a reason for hiding this comment

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

tldr AsyncEventDelegate, will look again when PR is ready

src/event/AsyncEvent.php Outdated Show resolved Hide resolved
src/event/AsyncEventTrait.php Outdated Show resolved Hide resolved
src/event/HandlerList.php Outdated Show resolved Hide resolved
src/event/HandlerList.php Show resolved Hide resolved
src/event/HandlerList.php Outdated Show resolved Hide resolved
src/event/ListenerMethodTags.php Outdated Show resolved Hide resolved
));
}
switch(strtolower($tags[ListenerMethodTags::NO_CONCURRENT_CALL])){
case "true":
Copy link
Member

Choose a reason for hiding this comment

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

since we have multiple of these, what about extracting to a function?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not directly related to this PR, it should be done externally.

src/plugin/PluginManager.php Outdated Show resolved Hide resolved
src/plugin/PluginManager.php Show resolved Hide resolved
break;
default:
throw new PluginException("Event handler " . Utils::getNiceClosureName($handlerClosure) . "() declares invalid @" . ListenerMethodTags::NO_CONCURRENT_CALL . " value \"" . $tags[ListenerMethodTags::NO_CONCURRENT_CALL] . "\"");
}
Copy link
Member

Choose a reason for hiding this comment

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

Not related to this PR, but both the API and this handler would be much nicer if we use attributes instead:

#[\pocketmine\event\AsyncHandler]
public function handler(XxxEvent $event) : Promise { ... }

#[\pocketmine\event\AsyncHandler(concurrent: true)]
public function handler(XxxEvent $event) : Promise { ... }

src/player/Player.php Outdated Show resolved Hide resolved
Co-authored-by: Javier León <58715544+JavierLeon9966@users.noreply.github.com>
Copy link
Member

@dktapps dktapps left a comment

Choose a reason for hiding this comment

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

It seems to me like a lot of complexity went into making async events and their handlers use the same frameworks that sync events do. The net effect of this is that async events are callable either synchronously or asynchronously, which seems to me like a design flaw.

src/event/Event.php Outdated Show resolved Hide resolved
@ShockedPlot7560
Copy link
Member Author

It seems to me like a lot of complexity went into making async events and their handlers use the same frameworks that sync events do. The net effect of this is that async events are callable either synchronously or asynchronously, which seems to me like a design flaw.

We can prohibit the synchronous call of a specified asynchronous event, but exiting the event will require a BC break every time we want to implement it in an event. As I said here synchronous and asynchronous calling is a subject for discussion, however, it's still necessary to keep synchronous handlers in the middle of synchronous ones, as not everyone needs to return a promise every time.

@ShockedPlot7560 ShockedPlot7560 marked this pull request as draft October 25, 2023 13:44
});
$listenersByPriority[$priority] = array_merge($listenersByPriority[$priority] ?? [], $asyncListeners);
}
$listenersByPriority = array_merge_recursive($listeners, $asyncListeners, $exclusiveAsyncListeners);
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't exclusive async listeners supposed to be run before normal async listeners?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the debate going on in my neurons. I don't have any pros or cons, really.

@ShockedPlot7560 ShockedPlot7560 marked this pull request as ready for review November 7, 2023 11:26
src/event/Event.php Outdated Show resolved Hide resolved
@dktapps
Copy link
Member

dktapps commented Nov 9, 2023

Promise::all() should be PR'd separately, as it's useful by itself and would reduce PR diff noise

@ShockedPlot7560
Copy link
Member Author

New behavior (again)

As async event is BC breaking by default, I detached the async event from the synchronous ones. Now, an async event should be handle with an async handler : must return a Promise. It can have a tag to specify if he need to be an exclusive handler or not.
Exclusive handlers will be called one by one, so no other handler can modify the event during the execution. Otherwise, concurrent handler will be call in parallel, priority by priority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: API Related to the plugin API Category: Core Related to internal functionality Type: Enhancement Contributes features or other improvements to PocketMine-MP
Projects
Events API
Async events
Development

Successfully merging this pull request may close these issues.

None yet

4 participants