Folia support

[cavallium]

Folia doesn't allow to use the Bukkit scheduler, it has four different scheduler types, tasks must be scheduled on the correct ones.

I created two implementations of BukkitService, since Bukkit/Spigot/Paper API and Folia API are mutually incompatible.

I marked the legacy scheduling methods as @Deprecated to make the tests compile, but they must be removed.

I made fallback implementations for all the new Folia scheduling methods in the legacy Bukkit/Spigot/Paper BukkitService.

I still haven't implemented the unit tests, I don't have time to re-implement most of them.

[cavallium]

Related issue: #2702

[games647]

First of all, thank you. Adding support for Folia would be great and this is already a big step forward.

Here are some of my opinions for discussion.

Breaking changes

At this stage there are a lot of breaking changes in your code. Either we should decide to drop support for them or add a fallback. For example:

teleportAsync is introduced by Paper. By adding the Folia API, it hides the unavailability of such methods from like Spigot. Maven multi modules could fix that, but could be overkill for this project. However, then requires such manual checks by the programmer and not be automatic by the compiler.

Thread-safety

Nevertheless, the biggest issue will be the topic around thread-safety. Before, we could expect that every statement scheduled from us or Bukkit to run on the main thread is not running concurrently with another. The introduction of multi region threading breaks this assumption.

Taking the examples for Folia project with some comments:
General rules of thumb:

Commands for entities/players are called on the region which owns the entity/player. Console commands are executed on the global region.

Implies: Commands invoked by players are now scattered across multi threads
Required action: Every statement accessing or changing AuthMe internal state have to introduce thread-safety mechanics.
Example: Verification command accessing shared HashSet that is not thread-safe. It could be accessed by multiple players on different regions.

Events involving a single entity (i.e player breaks/places block) are called on the region owning entity. Events involving actions on an entity (such as entity damage) are invoked on the region owning the target entity.

Similar to commands

Existing thread-safety assumptions

Similar what about existing thread-safe methods. AFAIK Player.sendMessage was declared as thread-safe. Do we now require that this method should accessed on the EntityScheduler? Taking our MessageTask should then no longer run asynchronous, but on this scheduler.

Proposal

All this likely requires huge refactoring, maybe separated into multiple PRs, before we could be safe to declare Folia support. The existing internal needs to be refactored to be thread-safe. For example locking/synchronizing the access on instance level, where possible.

Furthermore, it could be useful to add verification methods before we access anything player API (implemeneted as a Service?). So that we don't miss anything by accident. Something similar like Spigot already has for some API methods.

sendMessage(...) {
    // thread-safe API access
    if (currentThread == EntityScheduler || currentThread == player.getRegionThread()
}

It should be noted, that we still need to verify with the Spigot platform for potential deadlocks.

[cavallium]

teleportAsync is introduced by Paper. By adding the Folia API, it hides the unavailability of such methods from like Spigot. Maven multi modules could fix that, but could be overkill for this project. However, then requires such manual checks by the programmer and not be automatic by the compiler.

The API is commented with "// Paper start", "// Paper end" around every new API method, and the Spigot API javadocs are easily navigable, probably it's easier to manually check than overengineering everything.

[games647]

teleportAsync is introduced by Paper. By adding the Folia API, it hides the unavailability of such methods from like Spigot. Maven multi modules could fix that, but could be overkill for this project. However, then requires such manual checks by the programmer and not be automatic by the compiler.

The API is commented with "// Paper start", "// Paper end" around every new API method, and the Spigot API javadocs are easily navigable, probably it's easier to manually check than overengineering everything.

Not everybody is looking at the source code. You are more likely look at the auto-complete suggestions of your favorite IDE. With the Maven modules, at least it could be verified by the compiler.

[cavallium]

Existing thread-safety assumptions
Similar what about existing thread-safe methods. AFAIK Player.sendMessage was declared as thread-safe. Do we now require that this method should accessed on the EntityScheduler? Taking our MessageTask should then no longer run asynchronous, but on this scheduler.

Messages can be executed outside of any entity scheduler, since they do not interact with the world

[cavallium]

Furthermore, it could be useful to add verification methods before we access anything player API (implemeneted as a Service?). So that we don't miss anything by accident. Something similar like Spigot already has for some API methods.

sendMessage(...) {
    // thread-safe API access
    if (currentThread == EntityScheduler || currentThread == player.getRegionThread()
}

It should be noted, that we still need to verify with the Spigot platform for potential deadlocks.

Folia has already implemented this checks for us, so we could simply add that methods to the BukkitService

[cavallium]

I made the verification methods available. By the way, keep in mind that they are not so useful, because Folia already throws exceptions if you call any method outside of its scheduler.
Also, the old .scheduleSyncTaskFromOptionallyAsyncTask(...), replaced by their new counterparts, can overcome this limitation as you did in the past.

[games647]

Messages can be executed outside of any entity scheduler, since they do not interact with the world

Do you know if there is some kind of documentation about what is safe to be executed asynchronous? Spigot is such a huge project, it is hard to check if not a specific method accesses an internal component that isn't thread-safe. For example: The conversation and metadata (Player.setMetadata()) API. Either this has locking mechanism or it can only run on the entity scheduler.

I made the verification methods available. By the way, keep in mind that they are not so useful, because Folia already throws exceptions if you call any method outside of its scheduler.
Also, the old .scheduleSyncTaskFromOptionallyAsyncTask(...), replaced by their new counterparts, can overcome this limitation as you did in the past.

So something like this already exists? That would be perfect, because such thing is better suited to be in Folia anyway.

PlayerService {
  public void setHealth(...) {
    if (!currentThread == entityScheduler?/region thread?)
      throw new IllegalStateException("Wrong thread");
  }
}

The checks you implemented could be unnecessary if Folia is aware of which thread is currently itself is running on. In other words your checks (if globaltick then run.run()) could already be in Folia directly.

I'm sorry for all these questions, but I'm not that familiar with the project. Maybe it's also because Folia is still new and many things are planned.