-
-
Notifications
You must be signed in to change notification settings - Fork 395
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
Asynchronous event handling with functional non-thread-blocking chunk loading [PROPOSAL] #129
Asynchronous event handling with functional non-thread-blocking chunk loading [PROPOSAL] #129
Conversation
Many things to express. This change implements the EntityTeleportEvent in a context thread safe way. The following are handled: - Passengers have the event called properly - Recursion is prevented by checking for the teleport status. Such status is revoked by overriding and chaining the teleport callback. It is manually removed when the event fails and henceforth the callback would call null, or when the entities are in the same chunk region, and hence don't call the callback. - Teleporting status is removed on callback - Modifying the teleport location of passenger entities is forbidden to prevent conflicts - Cancelling an entity riding will dismount it and NOT trigger any events for its passengers
… bukkit event api - Reworked listeners. Now they execute synchronously after any edits including chunk loading. This allows for perfect asynchronous linear behaviour (tested!) - Reworked bukkit event api, introduced callEventAsync. If old method is call, the event is at risk of not having the proper data if paired up with any kind of chunk loading or non-direct modifier which is called asynchronously. This is the best we could possibly do without creating conflicting behaviour - Fixed teleportation whoopsie I made
…usly fetched thread context - If the current thread is not a region --> run consecutive code on current thread - If the origin of the event is unspecified --> heuristical approach at fetching current thread coords by taking a random coordinate in current map of thread. This can go wrong so easily, SpottedLeaf, this one's on you, I trust that you'll help me improve this - If the origin of the event is specified -> return to thread context at specified location (hurray) Additional improvements: - Made nms command /teleport depend on bukkit handling to support proper event handling - Added player teleportation to the handlers. All player teleports now have the same behaviour as chunk fetches (yipee) - Fixed a scenario of chunk loading I forgot about
Implemented the following events: - PlayerSpawnLocationEvent, - EntityPortalEnterEvent, - EntityPortalExitEvent, - PlayerPortalEvent, - EntityPortalReadyEvent
cool,a new anticheat? |
touché |
…hread and offthread events - Fixed issue where global thread events wouldn't be properly accounted for - Fixed an issue where the EntityEnterPortalEvent would be fired every tick (god knows how long this has been there like this)
This is great |
@Spottedleaf If you ever get time to discuss this proposal lmk. I’d love to put more effort into it, although I’d like to preferably see some sort of proper nod up so my efforts don’t end up being trashed yknow :) |
They may ask you to follow paper conventions (dont change nms file imports & proper comments) |
Duly noted, will make the appropriate changes to follow these |
- Fix non-region threads calling teleport and chunk functions failing - Fixed wrong origin level in player and entity portal event
I think it's very important that if you are going to create a large PR that you do not write anything before having the concepts approved beforehand, as it is easiest to change things in design rather than in code. As it stands, the design concepts in this PR would not have been approved, which would have saved you a lot of effort on your part. The reason the events for teleportation/spawning/other are absent is because the required implementation is requires an entirely different event system. I haven't had the time to dedicate to making one, or even to design one abstractly, and certainly not to write any documentation on the matter. Considering the PR though, there are simply too many problems for it to be merged in its current state or even a modified state. These issues are as follows:
The new event system must allow listeners to decide when the next listener is invoked. This is to allow the listener to use any thread context it wants. However, this approach has numerous issues: plugins never forwarding to the next listener, plugins taking too long to forward to the next listener, and undefined thread context for which listeners are invoked. These issues are solvable, as it just needs proper design. I would suspect that bringing this system to Paper is desirable, which means that it would need additional approval of other Paper maintainers. Additionally, the event system must not use CompletableFuture in any shape or form. This is due to CompletableFuture's exception handling being completely atrocious, and for general API it is not acceptable to have such poor exception handling. It is extremely easy to fall into a trap where exceptions are not handled or printed. This PR is a relevant example, as it does not perform proper exception handling (print, rethrow, or handle) in many areas. I have learned the hard way from the Vanilla chunk system that CompletableFuture is nothing but a complete pain when debugging needs to be done, and to this day I run into issues with CompletableFuture usage not printing exceptions. It is not worth using them to design a larger system. Your hooks into getChunkAtAsync and teleportAsync are incorrect. The CompletableFuture returned by the original function is going to be completed when the chunk or entity is teleported. Then, your code will remove that future from the observer list and return a future that is completed after that is done. So, any plugin logic that hooks onto that future can only run after the observer is removed. This actually results in the plugin logic being delayed until after all of the remaining listeners are invoked - by that time, the chunk may be unloaded. More importantly, the thread context may have switched and operations on chunks by the wrong thread context will throw exceptions. And finally, it will violate the event firing order. As a result, while it may appear in very limited scenarios that your implementation works it is fundamentally flawed. An additional note on CompletableFutures is that the execution context for callbacks is completely undefined. This is an additional reason that CompletableFuture is unacceptable to use, as it is has these gotchas on top of the debugging issues. Let's use the example in your code: event.getPlayer().getWorld().getChunkAtAsync(event.getTo().add(100, 0, 100)).thenRun(() -> {
System.out.println("Chunk loaded");
event.getTo().set(100, 100, 100);
}); On what thread context does the Runnable passed to thenRun() run if we exclude the logic this PR adds? There are actually three answers:
The 3rd one is not immediately obvious when it happens and in fact only rarely happens. This is due to a race condition with the API design using CompletableFuture. The load is scheduled to happen asynchronously, which will then complete the returned future. However, it is possible for the future to be completed on another region before thenRun() is invoked. In such a case, thenRun() is invoked after the future is completed which will then invoke the Runnable synchronously. As a result, a plugin using getChunkAtAsync cannot even guarantee that it can use the chunk unless it invokes getChunkAtAsync on the right thread context to begin with. This issue is entirely attributable to the use of CompletableFuture. The API should have never been designed to use CompletableFuture in the first place due to this behavior. But, we cannot just delete this API and so that's that. As a result of these major issues, I will not be taking this PR in this form. I don't see what can be used from this PR to meet the requirements for the event system changes. Right now, this area of code is not a high priority for me to look into. I would consider the world loading stuff to be more important. Even then, the world loading stuff is an order of magnitude more complicated than this. For larger PRs you should seek approval on the general design before writing anything to ensure you don't start writing code for something that wont be approved. |
Asynchronous event handling proposal
What is this proposal? Well, as Folia keeps growing in popularity, so does the demand for its plugins. However, one conflicting issue regarding movement events has constantly been subject of controversy: getting chunks for positions included in the event.
This specific issue targets Teleport and dimensions changing events, notable the Teleport Event. This event is crucial for many reasons, and as such it is important to consider it.
0. Roadmap
Event implementation
1. Philosophy
The main philosophy and optic I am aiming towards can be summarised in three points:
With all of this in mind, how can we effectively allow users to use functions as such:
With code as above, well, since the event is asynchronous, the following flow will happen:
Race condition
![Untitled-2023-07-11-0416]()But we don't want this! We want this:
Desired computation
Problem: How can we "guess" when users are getting asynchronous tasks from the server and how can we make sure we always get the end result.
Well, turns out, we are the server, and CompletableFutures are amazing.
2. Proposed Solution
To circumvent these issues, we must listen to every single endpoint which requests our chunks asynchronously. Lucky enough for us, it's literally one function.
Hence, the flow goes like this:
a. If another event is fired, the current event waits for the fired event to be completed. This maintains nested synchronousy
3. Implementation
For this implementation, we add the following class:
EventTaskHandler.java
Additions to TickRegionScheduler.java
PaperEventManager.java
Example event: PlayerTeleportEvent / EntityTeleportEvent
Many things to express. This change implements the EntityTeleportEvent in a context thread safe way. The following are handled:
Modified Entity.java class
4 Proof of concept (?)
https://www.youtube.com/watch?v=dLGMj84XsSk
5. Drawbacks and unimplemented concepts
Obviously, as of right now, every listener is executed linearly and the result changes reflect the such. Except, this is bad practice. We want to follow an order for listeners... That's the point of Event driven architecture.The changes required would effectively take the listeners and independently run them through in this system. Instead of clearing post event, we would clear and handle changes to the event after every listener. This would ensure linearity and would safeguard the current event system.Nvm fixed lmao I can't think of one drawback besides nagging devs to properly update their apis to call events async if folia is activated