From 217ef7d8209b5a04a90548c2d9b2733a377c462c Mon Sep 17 00:00:00 2001 From: twisti-dev <76837088+twisti-dev@users.noreply.github.com> Date: Sat, 17 Oct 2026 13:42:35 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20feat(minestom):=20add=20server=20li?= =?UTF-8?q?fecycle,=20surf=20player=20and=20runtime=20plugin=20downloads?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - enable features and plugins synchronously in surfMinestomServer, before the server accepts players; a failing enable fails the start and rolls back - refuse a shutdown from the starting thread and fail the start when another thread requests one - add SurfMinestomServer.stop(), which stops Minestom, the api and then the onShutdown actions once, lets concurrent callers wait and refuses tick threads - add builder onShutdown actions and a JVM shutdown hook installed after a successful start - set minestom.shutdown-on-signal=false in the bootstrap unless it is given with -D - add SurfMinestomPlayer with the Adventure permission pointer, vanilla-like game mode switches, signed chat sessions in player list entries and shared viewable rules - add builder playerFactory; ConnectionManagerMixin rejects other player providers while the api runs, and a provider set before the start fails it outside of tests - add PlayerVisibility.isManaged and Provider.manages for the viewable rule of managed players - add withPlugins { requireAll }, which also counts unreadable plugin descriptions, and create plugins/ before a plugin is constructed - start the console reader on the first tick and stop the server from its default onShutdown - add downloadSurfModules and the shaded configuration to the minestom-server Gradle plugin - let SurfMixinAgent read the server jar reported by the bootstrap, so it works when surf-api modules are downloaded - use stop() in the example server and log from an onShutdown action - bump version to 3.49.0 and the Gradle plugin to 2.5.0 --- gradle.properties | 2 +- surf-api-gradle-plugin/build.gradle.kts | 2 +- .../server/MinestomServerSurfExtension.kt | 31 +- .../server/MinestomServerSurfPlugin.kt | 104 +++++-- .../server/MinestomServerSurfPluginTest.kt | 39 +++ .../api/minestom/example/ExampleCommands.kt | 20 +- .../api/minestom/example/ExampleServer.kt | 3 + .../bootstrap/SurfMinestomBootstrap.java | 41 ++- .../minestom/server/console/ConsoleFeature.kt | 14 +- ...-api-minestom-server-player-visibility.api | 1 + .../visibility/PlayerVisibilityFeature.kt | 2 + .../visibility/PlayerVisibilityHandler.kt | 9 +- .../api/surf-api-minestom-server-plugins.api | 2 + .../minestom/server/plugins/PluginsFeature.kt | 47 ++- .../server/plugins/impl/PluginCandidate.kt | 22 +- .../server/plugins/impl/PluginManagerImpl.kt | 49 +++- .../server/plugins/PluginsFeatureTest.kt | 11 +- .../server/plugins/PluginsRequireAllTest.kt | 233 +++++++++++++++ .../server/plugins/RequireAllTestPlugins.kt | 60 ++++ .../api/surf-api-minestom-server.api | 12 + .../server/impl/mixin/SurfMixinAgent.java | 60 ++-- .../server/mixin/ConnectionManagerMixin.java | 17 +- .../api/minestom/server/SurfMinestomServer.kt | 86 +++++- .../server/impl/SurfMinestomRuntime.kt | 153 ++++++++-- .../server/impl/player/SurfPlayerProviders.kt | 83 ++++++ .../minestom/server/SurfMinestomServerTest.kt | 185 +++++++++++- .../impl/player/SurfPlayerProvidersTest.kt | 89 ++++++ .../api/surf-api-minestom.api | 11 + .../api/minestom/player/SurfMinestomPlayer.kt | 159 ++++++++++ .../api/minestom/plugin/SurfMinestomPlugin.kt | 11 +- .../minestom/visibility/PlayerVisibility.kt | 10 + .../minestom/player/SurfMinestomPlayerTest.kt | 274 ++++++++++++++++++ 32 files changed, 1702 insertions(+), 140 deletions(-) create mode 100644 surf-api-gradle-plugin/src/test/kotlin/dev/slne/surf/api/gradle/platform/minestom/server/MinestomServerSurfPluginTest.kt create mode 100644 surf-api-minestom/surf-api-minestom-server-plugins/src/test/kotlin/dev/slne/surf/api/minestom/server/plugins/PluginsRequireAllTest.kt create mode 100644 surf-api-minestom/surf-api-minestom-server-plugins/src/test/kotlin/dev/slne/surf/api/minestom/server/plugins/RequireAllTestPlugins.kt create mode 100644 surf-api-minestom/surf-api-minestom-server/src/main/kotlin/dev/slne/surf/api/minestom/server/impl/player/SurfPlayerProviders.kt create mode 100644 surf-api-minestom/surf-api-minestom-server/src/test/kotlin/dev/slne/surf/api/minestom/server/impl/player/SurfPlayerProvidersTest.kt create mode 100644 surf-api-minestom/surf-api-minestom/src/main/kotlin/dev/slne/surf/api/minestom/player/SurfMinestomPlayer.kt create mode 100644 surf-api-minestom/surf-api-minestom/src/test/kotlin/dev/slne/surf/api/minestom/player/SurfMinestomPlayerTest.kt diff --git a/gradle.properties b/gradle.properties index e808244dd..a56a8eb5e 100644 --- a/gradle.properties +++ b/gradle.properties @@ -7,6 +7,6 @@ org.jetbrains.dokka.experimental.gradle.pluginMode=V2Enabled javaVersion=25 mcVersion=26.3 group=dev.slne.surf.api -version=3.48.1 +version=3.49.0 relocationPrefix=dev.slne.surf.api.libs snapshot=false diff --git a/surf-api-gradle-plugin/build.gradle.kts b/surf-api-gradle-plugin/build.gradle.kts index 490f56517..406bb3906 100644 --- a/surf-api-gradle-plugin/build.gradle.kts +++ b/surf-api-gradle-plugin/build.gradle.kts @@ -20,7 +20,7 @@ plugins { group = groupId version = buildString { - append("2.4.1") + append("2.5.0") if (snapshot) append("-SNAPSHOT") } diff --git a/surf-api-gradle-plugin/src/main/kotlin/dev/slne/surf/api/gradle/platform/minestom/server/MinestomServerSurfExtension.kt b/surf-api-gradle-plugin/src/main/kotlin/dev/slne/surf/api/gradle/platform/minestom/server/MinestomServerSurfExtension.kt index 1c1381154..52c897cd3 100644 --- a/surf-api-gradle-plugin/src/main/kotlin/dev/slne/surf/api/gradle/platform/minestom/server/MinestomServerSurfExtension.kt +++ b/surf-api-gradle-plugin/src/main/kotlin/dev/slne/surf/api/gradle/platform/minestom/server/MinestomServerSurfExtension.kt @@ -54,9 +54,21 @@ enum class MinestomServerFeature( * Calling `withXxx()` only puts the feature's module on the class path; the server still installs * it in `surfMinestomServer { }`. * - * By default, the server jar only contains the project's own code and the surf modules. Minestom - * and every other third-party library are downloaded into `libraries/` when the server starts, see - * [downloadLibraries]. + * By default, the server jar only contains the project's own code, the surf modules and the + * dependencies declared in the `shaded` configuration. Minestom and every other third-party + * library are downloaded into `libraries/` when the server starts, see [downloadLibraries]. With + * [downloadSurfModules], the surf modules are downloaded as well. + * + * Dependencies from repositories that need credentials cannot be downloaded at runtime. Declare + * them in the `shaded` configuration to embed them in the server jar: + * ``` + * dependencies { + * shaded(libs.private.plugin) + * } + * ``` + * The whole resolved closure of a shaded dependency is embedded and therefore not downloaded, which + * may include public modules it depends on. `isTransitive = false` limits a declaration to the + * module itself. */ open class MinestomServerSurfExtension @Inject constructor(objects: ObjectFactory) : CoreSurfExtension(objects) { @@ -69,10 +81,21 @@ open class MinestomServerSurfExtension @Inject constructor(objects: ObjectFactor * Whether third-party libraries are downloaded with gremlin when the server starts instead of * being shaded into the server jar. They are fetched from the project's HTTP(S) Maven * repositories, so libraries that only exist in `mavenLocal()` or need credentials have to be - * shaded. Surf modules (`dev.slne.surf*`) are always shaded. + * declared in the `shaded` configuration. Surf modules (`dev.slne.surf*`) are shaded unless + * [downloadSurfModules] is set. */ val downloadLibraries = objects.property().convention(true) + /** + * Whether surf modules (`dev.slne.surf*`, such as surf-api and the bundled plugins) are + * downloaded on the first start like third-party libraries instead of being shaded. + * + * Only the project's own modules, the bootstrap and the `shaded` dependencies stay in the + * server jar, which keeps it small. Requires [downloadLibraries]; without it the setting has no + * effect and every module is shaded. + */ + val downloadSurfModules = objects.property().convention(false) + /** * The plugins from the `plugins` directory the server's own code uses, by ID. * diff --git a/surf-api-gradle-plugin/src/main/kotlin/dev/slne/surf/api/gradle/platform/minestom/server/MinestomServerSurfPlugin.kt b/surf-api-gradle-plugin/src/main/kotlin/dev/slne/surf/api/gradle/platform/minestom/server/MinestomServerSurfPlugin.kt index 2aa6bf26e..9329255f7 100644 --- a/surf-api-gradle-plugin/src/main/kotlin/dev/slne/surf/api/gradle/platform/minestom/server/MinestomServerSurfPlugin.kt +++ b/surf-api-gradle-plugin/src/main/kotlin/dev/slne/surf/api/gradle/platform/minestom/server/MinestomServerSurfPlugin.kt @@ -12,8 +12,10 @@ import org.gradle.api.Project import org.gradle.api.artifacts.ResolvedDependency import org.gradle.api.artifacts.component.ModuleComponentIdentifier import org.gradle.api.artifacts.component.ProjectComponentIdentifier +import org.gradle.api.attributes.Attribute import org.gradle.api.file.DuplicatesStrategy import org.gradle.api.plugins.JavaApplication +import org.gradle.api.provider.Provider import org.gradle.api.tasks.Exec import org.gradle.api.tasks.SourceSet import org.gradle.api.tasks.SourceSetContainer @@ -29,11 +31,18 @@ import xyz.jpenilla.gremlin.gradle.WriteDependencySet * Builds a runnable surf Minestom server as one executable server jar: the surf api, the * requested features and the project's own code. * - * Only the project's own modules, the surf modules and the bootstrap are shaded. Every other - * library is listed in the gremlin dependency set `dependencies.txt`. The bootstrap agent + * The server jar contains the project's own modules, the bootstrap, the surf modules and the + * modules declared in the `shaded` configuration. With + * [MinestomServerSurfExtension.downloadSurfModules] the surf modules are downloaded instead. Every + * other library is listed in the gremlin dependency set `dependencies.txt`. The bootstrap agent * downloads these libraries into `libraries/`, relocates them like the server jar and appends * them to the system class path before `main` runs. * + * The `shaded` configuration is extended by `runtimeOnly` and embeds modules that cannot be + * downloaded, such as those of repositories that need credentials. The whole resolved closure of + * the dependencies declared in it is embedded, including public modules they depend on. A module + * of that closure is never downloaded, regardless of other paths to it. + * * Manifest attributes written to the jar: * - `Launcher-Agent-Class`: the bootstrap agent. * - `Surf-Delegate-Agent-Classes`: delegate agents the bootstrap agent calls after the libraries @@ -68,10 +77,18 @@ internal class MinestomServerSurfPlugin : } override fun Project.configure0() { + val shaded = configurations.create(SHADED) { + description = "Dependencies embedded in the server jar instead of being downloaded, " + + "for repositories that need credentials" + isCanBeConsumed = false + isCanBeResolved = true + } + configurations.named("runtimeOnly") { extendsFrom(shaded) } + dependencies { - add("implementation", Constants.MINESTOM) - add("runtimeOnly", "org.apache.logging.log4j:log4j-core:${Constants.LOG4J_VERSION}") - add("runtimeOnly", "org.apache.logging.log4j:log4j-slf4j2-impl:${Constants.LOG4J_VERSION}") + "implementation"(Constants.MINESTOM) + "runtimeOnly"("org.apache.logging.log4j:log4j-core:${Constants.LOG4J_VERSION}") + "runtimeOnly"("org.apache.logging.log4j:log4j-slf4j2-impl:${Constants.LOG4J_VERSION}") } tasks.named("shadowJar") { @@ -137,6 +154,9 @@ internal class MinestomServerSurfPlugin : if (extension.downloadLibraries.get()) { configureLibraryDownload(extension) } else { + if (extension.downloadSurfModules.get()) { + logger.warn("downloadSurfModules has no effect on ${project.path} while downloadLibraries is disabled") + } tasks.named("processResources") { exclude(DEPENDENCIES_FILE) } configureShadedAsmVersion() } @@ -202,11 +222,33 @@ internal class MinestomServerSurfPlugin : } /** - * Writes every third-party library, with the server jar's relocations, to `dependencies.txt`, - * the file the bootstrap downloads from. The project's own modules, the surf modules and the - * bootstrap stay shaded in the server jar. + * Writes every library that is not shaded, with the server jar's relocations, to + * `dependencies.txt`, the file the bootstrap downloads from. The [ShadingRules] decide which + * modules stay in the server jar. */ private fun Project.configureLibraryDownload(extension: MinestomServerSurfExtension) { + val runtimeClasspath = configurations.getByName("runtimeClasspath") + val shaded = configurations.named(SHADED) { + // Selects the same versions as the runtime graph and resolves the same variants + shouldResolveConsistentlyWith(runtimeClasspath) + attributes { + runtimeClasspath.attributes.keySet().forEach { key -> + @Suppress("UNCHECKED_CAST") + attribute(key as Attribute, runtimeClasspath.attributes.getAttribute(key)!!) + } + } + } + val rules = ShadingRules( + downloadSurfModules = extension.downloadSurfModules.get(), + embedded = shaded.map { configuration -> + configuration.incoming.resolutionResult.allComponents + .mapNotNull { component -> + (component.id as? ModuleComponentIdentifier)?.let { it.group to it.module } + } + .toSet() + }, + ) + configurations.named(RUNTIME_DOWNLOAD) { extendsFrom(configurations.getByName("implementation"), configurations.getByName("runtimeOnly")) shouldResolveConsistentlyWith(configurations.getByName("runtimeClasspath")) @@ -215,8 +257,10 @@ internal class MinestomServerSurfPlugin : tasks.named("writeDependencies") { // Filtered per component, so that the libraries of shaded modules are still downloaded dependencies.setFrom(configurations.named(RUNTIME_DOWNLOAD).map { configuration -> + // Resolves `shaded` before the artifact view, not during its resolution + rules.embeddedModules configuration.incoming.artifactView { - componentFilter { id -> id !is ModuleComponentIdentifier || !isShaded(id.group, id.module) } + componentFilter { id -> id !is ModuleComponentIdentifier || !rules.isShaded(id.group, id.module) } }.artifacts }) forEachRelocation { from, to, excludes -> @@ -232,7 +276,7 @@ internal class MinestomServerSurfPlugin : tasks.named("shadowJar") { dependencies { - exclude { dependency -> !dependency.isShaded() } + exclude { dependency -> !rules.isShaded(dependency) } } } } @@ -263,16 +307,35 @@ internal class MinestomServerSurfPlugin : } } - private fun ResolvedDependency.isShaded() = - isShaded(moduleGroup, moduleName) || - moduleArtifacts.any { it.id.componentIdentifier is ProjectComponentIdentifier } - /** - * Whether the module stays in the server jar: surf modules (not the third-party forks under - * `dev.slne.forks`) and the [BOOTSTRAP_MODULES]. + * Decides which modules stay in the server jar when the libraries are downloaded. + * + * A module is shaded when it is one of the [BOOTSTRAP_MODULES], a project dependency, part of + * the `shaded` closure or, unless [downloadSurfModules] is set, a surf module. Surf modules + * exclude the third-party forks under `dev.slne.forks`. Every component therefore has exactly + * one destination: the server jar or `dependencies.txt`. + * + * The `shaded` closure is resolved on first use, at execution time, and then kept. + * + * @param embedded `group` to `name` of every module in the resolution result of `shaded` */ - private fun isShaded(group: String, module: String) = - group == "dev.slne.surf" || group.startsWith("dev.slne.surf.") || group to module in BOOTSTRAP_MODULES + private class ShadingRules( + private val downloadSurfModules: Boolean, + embedded: Provider>>, + ) { + val embeddedModules: Set> by lazy { embedded.get() } + + fun isShaded(group: String, module: String): Boolean { + val id = group to module + return id in BOOTSTRAP_MODULES || + id in embeddedModules || + (!downloadSurfModules && (group == "dev.slne.surf" || group.startsWith("dev.slne.surf."))) + } + + fun isShaded(dependency: ResolvedDependency) = + isShaded(dependency.moduleGroup, dependency.moduleName) || + dependency.moduleArtifacts.any { it.id.componentIdentifier is ProjectComponentIdentifier } + } /** * Registers `runServer`, which runs the server jar with `java -jar` so that the manifest's @@ -306,6 +369,7 @@ internal class MinestomServerSurfPlugin : const val LOG4J_CONFIG = "log4j2.xml" const val RUNTIME_DOWNLOAD = "runtimeDownload" + const val SHADED = "shaded" const val JAR_RELOCATOR_RUNTIME = "jarRelocatorRuntime" const val DEPENDENCIES_FILE = "dependencies.txt" const val BOOTSTRAP_MODULE = "surf-api-minestom-server-bootstrap" @@ -317,9 +381,11 @@ internal class MinestomServerSurfPlugin : const val MIXIN_CONFIG = "mixins.surf-api-minestom-server.json" /** - * What the bootstrap needs before anything is downloaded. + * What the bootstrap needs before anything is downloaded, independent of + * [MinestomServerSurfExtension.downloadSurfModules]. */ val BOOTSTRAP_MODULES = setOf( + "dev.slne.surf.api" to BOOTSTRAP_MODULE, "xyz.jpenilla" to "gremlin-runtime", "org.jspecify" to "jspecify", ) diff --git a/surf-api-gradle-plugin/src/test/kotlin/dev/slne/surf/api/gradle/platform/minestom/server/MinestomServerSurfPluginTest.kt b/surf-api-gradle-plugin/src/test/kotlin/dev/slne/surf/api/gradle/platform/minestom/server/MinestomServerSurfPluginTest.kt new file mode 100644 index 000000000..daf76d754 --- /dev/null +++ b/surf-api-gradle-plugin/src/test/kotlin/dev/slne/surf/api/gradle/platform/minestom/server/MinestomServerSurfPluginTest.kt @@ -0,0 +1,39 @@ +package dev.slne.surf.api.gradle.platform.minestom.server + +import org.gradle.kotlin.dsl.getByType +import org.gradle.testfixtures.ProjectBuilder +import kotlin.test.Test +import kotlin.test.assertFalse +import kotlin.test.assertNotNull +import kotlin.test.assertTrue + +class MinestomServerSurfPluginTest { + + private val project = ProjectBuilder.builder().build().also { + it.plugins.apply(MinestomServerSurfPlugin::class.java) + } + + @Test + fun `shaded configuration is resolvable but not consumable`() { + val shaded = project.configurations.getByName("shaded") + + assertTrue(shaded.isCanBeResolved) + assertFalse(shaded.isCanBeConsumed) + assertNotNull(shaded.description) + } + + @Test + fun `runtimeOnly extends shaded`() { + val shaded = project.configurations.getByName("shaded") + + assertTrue(shaded in project.configurations.getByName("runtimeOnly").extendsFrom) + } + + @Test + fun `downloadSurfModules is disabled by default and downloadLibraries enabled`() { + val extension = project.extensions.getByType() + + assertFalse(extension.downloadSurfModules.get()) + assertTrue(extension.downloadLibraries.get()) + } +} diff --git a/surf-api-minestom/surf-api-minestom-example/src/main/kotlin/dev/slne/surf/api/minestom/example/ExampleCommands.kt b/surf-api-minestom/surf-api-minestom-example/src/main/kotlin/dev/slne/surf/api/minestom/example/ExampleCommands.kt index cc9862f40..fbd133980 100644 --- a/surf-api-minestom/surf-api-minestom-example/src/main/kotlin/dev/slne/surf/api/minestom/example/ExampleCommands.kt +++ b/surf-api-minestom/surf-api-minestom-example/src/main/kotlin/dev/slne/surf/api/minestom/example/ExampleCommands.kt @@ -1,23 +1,17 @@ package dev.slne.surf.api.minestom.example -import dev.slne.surf.api.minestom.command.dsl.anyExecutor -import dev.slne.surf.api.minestom.command.dsl.commandAPICommand -import dev.slne.surf.api.minestom.command.dsl.integerArgument -import dev.slne.surf.api.minestom.command.dsl.playerArgument -import dev.slne.surf.api.minestom.command.dsl.playerExecutor -import dev.slne.surf.api.minestom.command.dsl.playerExecutorSuspend -import dev.slne.surf.api.minestom.command.dsl.signedMessageArgument -import dev.slne.surf.api.minestom.command.dsl.subcommand +import dev.slne.surf.api.minestom.command.dsl.* import dev.slne.surf.api.minestom.coroutine.withEntity import dev.slne.surf.api.minestom.highlight.BlockHighlights +import dev.slne.surf.api.minestom.server.SurfMinestomServer import dev.slne.surf.api.minestom.visibility.PlayerVisibility import kotlinx.coroutines.delay import net.kyori.adventure.chat.SignedMessage import net.kyori.adventure.text.Component import net.kyori.adventure.text.format.NamedTextColor -import net.minestom.server.MinecraftServer import net.minestom.server.entity.GameMode import net.minestom.server.entity.Player +import kotlin.system.exitProcess import kotlin.time.Duration.Companion.seconds /** @@ -70,9 +64,15 @@ internal object ExampleCommands { } } + // stop() waits until the plugins are disabled, which a tick thread must not wait for commandAPICommand("stop") { withPermission("example.stop") - anyExecutor { _, _ -> MinecraftServer.stopCleanly() } + anyExecutor { _, _ -> + Thread({ + SurfMinestomServer.instance.stop() + exitProcess(0) + }, "example-stop").start() + } } // The message keeps the signature the client produced for the argument diff --git a/surf-api-minestom/surf-api-minestom-example/src/main/kotlin/dev/slne/surf/api/minestom/example/ExampleServer.kt b/surf-api-minestom/surf-api-minestom-example/src/main/kotlin/dev/slne/surf/api/minestom/example/ExampleServer.kt index c0afaf1d2..7d000d456 100644 --- a/surf-api-minestom/surf-api-minestom-example/src/main/kotlin/dev/slne/surf/api/minestom/example/ExampleServer.kt +++ b/surf-api-minestom/surf-api-minestom-example/src/main/kotlin/dev/slne/surf/api/minestom/example/ExampleServer.kt @@ -76,6 +76,9 @@ fun main() { maxPlayers = 20 listeners(ExampleListeners(world)) + + // Runs after the plugins and features are disabled, also when the process is terminated + onShutdown { LOGGER.info("Example server stopped") } } ExampleCommands.register() diff --git a/surf-api-minestom/surf-api-minestom-server-bootstrap/src/main/java/dev/slne/surf/api/minestom/server/bootstrap/SurfMinestomBootstrap.java b/surf-api-minestom/surf-api-minestom-server-bootstrap/src/main/java/dev/slne/surf/api/minestom/server/bootstrap/SurfMinestomBootstrap.java index 6774a3280..d60f30816 100644 --- a/surf-api-minestom/surf-api-minestom-server-bootstrap/src/main/java/dev/slne/surf/api/minestom/server/bootstrap/SurfMinestomBootstrap.java +++ b/surf-api-minestom/surf-api-minestom-server-bootstrap/src/main/java/dev/slne/surf/api/minestom/server/bootstrap/SurfMinestomBootstrap.java @@ -55,6 +55,13 @@ public final class SurfMinestomBootstrap { */ public static final String LIBRARIES_PROPERTY = "surf.minestom.libraries"; + /** + * The system property the bootstrap agent sets to the path of the server jar before it calls + * the delegate agents, so that an agent loaded from a downloaded library still finds the + * manifest of the server jar. + */ + public static final String SERVER_JAR_PROPERTY = "surf.minestom.server-jar"; + /** * The Minestom server properties that may change at runtime, see * {@code SurfMinestomServerBuilder.recommendedServerProperties}. Minestom freezes a property's @@ -62,6 +69,13 @@ public final class SurfMinestomBootstrap { */ private static final List MUTABLE_SERVER_PROPERTIES = List.of("minestom.keep-alive-delay"); + /** + * Minestom's own JVM shutdown hook. The surf api stops the server from a hook of its own, which + * also runs the server's shutdown actions, so Minestom's is turned off unless set with + * {@code -D}. + */ + private static final String SHUTDOWN_ON_SIGNAL_PROPERTY = "minestom.shutdown-on-signal"; + private static final BootstrapLogger LOGGER = new BootstrapLogger(); private SurfMinestomBootstrap() { @@ -77,8 +91,14 @@ public static void agentmain(final @Nullable String args, final Instrumentation System.setProperty(property + ".mutable", "true"); } } + if (System.getProperty(SHUTDOWN_ON_SIGNAL_PROPERTY) == null) { + System.setProperty(SHUTDOWN_ON_SIGNAL_PROPERTY, "false"); + } + + final Path serverJar = ownJar(); + System.setProperty(SERVER_JAR_PROPERTY, serverJar.toString()); - final Attributes manifest = ownManifest(); + final Attributes manifest = manifest(serverJar); installLibraries(instrumentation); final @Nullable String serverPlugins = manifest.getValue(ServerPlugins.ATTRIBUTE); @@ -160,14 +180,19 @@ private static void callAgent(final String className, final @Nullable String arg } } - private static Attributes ownManifest() { + private static Path ownJar() { try { - final Path jar = Path.of(SurfMinestomBootstrap.class.getProtectionDomain().getCodeSource().getLocation().toURI()); - try (JarFile jarFile = new JarFile(jar.toFile())) { - final @Nullable Manifest manifest = jarFile.getManifest(); - return manifest == null ? new Attributes() : manifest.getMainAttributes(); - } - } catch (final IOException | URISyntaxException e) { + return Path.of(SurfMinestomBootstrap.class.getProtectionDomain().getCodeSource().getLocation().toURI()); + } catch (final URISyntaxException e) { + throw new IllegalStateException("Failed to locate the server jar", e); + } + } + + private static Attributes manifest(final Path jar) { + try (JarFile jarFile = new JarFile(jar.toFile())) { + final @Nullable Manifest manifest = jarFile.getManifest(); + return manifest == null ? new Attributes() : manifest.getMainAttributes(); + } catch (final IOException e) { throw new IllegalStateException("Failed to read the manifest of the server jar", e); } } diff --git a/surf-api-minestom/surf-api-minestom-server-console/src/main/kotlin/dev/slne/surf/api/minestom/server/console/ConsoleFeature.kt b/surf-api-minestom/surf-api-minestom-server-console/src/main/kotlin/dev/slne/surf/api/minestom/server/console/ConsoleFeature.kt index 8f06da31e..cd5d9f56e 100644 --- a/surf-api-minestom/surf-api-minestom-server-console/src/main/kotlin/dev/slne/surf/api/minestom/server/console/ConsoleFeature.kt +++ b/surf-api-minestom/surf-api-minestom-server-console/src/main/kotlin/dev/slne/surf/api/minestom/server/console/ConsoleFeature.kt @@ -1,5 +1,6 @@ package dev.slne.surf.api.minestom.server.console +import dev.slne.surf.api.minestom.extension.SchedulerManager import dev.slne.surf.api.minestom.server.SurfMinestomFeature import dev.slne.surf.api.minestom.server.SurfMinestomServer import dev.slne.surf.api.minestom.server.SurfMinestomServerBuilder @@ -20,12 +21,14 @@ class ConsoleFeatureBuilder internal constructor() { var threadName: String = "surf-minestom-console" /** - * Runs when the console asks the server to stop, e.g. on Ctrl+C or end of input. + * Runs on the console thread when the console asks the server to stop, e.g. on Ctrl+C or end + * of input. * - * Stops the server and exits the process by default. + * Stops the server with [SurfMinestomServer.stop], which also runs its shutdown actions, and + * exits the process by default. */ var onShutdown: () -> Unit = { - MinecraftServer.stopCleanly() + SurfMinestomServer.instance.stop() exitProcess(0) } } @@ -52,10 +55,11 @@ class ConsoleFeature internal constructor( override val id: String = ID override fun enable(api: SurfMinestomServer) { - Thread(SurfTerminalConsole(settings.onShutdown)::start, settings.threadName).apply { + val console = Thread(SurfTerminalConsole(settings.onShutdown)::start, settings.threadName).apply { isDaemon = true - start() } + + SchedulerManager.scheduleNextTick(console::start) } override fun disable(api: SurfMinestomServer) { diff --git a/surf-api-minestom/surf-api-minestom-server-player-visibility/api/surf-api-minestom-server-player-visibility.api b/surf-api-minestom/surf-api-minestom-server-player-visibility/api/surf-api-minestom-server-player-visibility.api index 9d5cef3d4..ecfb1eff5 100644 --- a/surf-api-minestom/surf-api-minestom-server-player-visibility/api/surf-api-minestom-server-player-visibility.api +++ b/surf-api-minestom/surf-api-minestom-server-player-visibility/api/surf-api-minestom-server-player-visibility.api @@ -7,6 +7,7 @@ public final class dev/slne/surf/api/minestom/server/visibility/PlayerVisibility public fun getLoadPriority ()I public fun isLimited (Lnet/minestom/server/entity/Player;)Z public fun load (Ldev/slne/surf/api/minestom/server/SurfMinestomServer;)V + public fun manages (Lnet/minestom/server/entity/Player;)Z public fun refreshViewerRule (Lnet/minestom/server/entity/Player;)Z public fun updateViewerRule (Lnet/minestom/server/entity/Player;Ljava/util/function/Predicate;)Z } diff --git a/surf-api-minestom/surf-api-minestom-server-player-visibility/src/main/kotlin/dev/slne/surf/api/minestom/server/visibility/PlayerVisibilityFeature.kt b/surf-api-minestom/surf-api-minestom-server-player-visibility/src/main/kotlin/dev/slne/surf/api/minestom/server/visibility/PlayerVisibilityFeature.kt index 775cb704a..5aa86d139 100644 --- a/surf-api-minestom/surf-api-minestom-server-player-visibility/src/main/kotlin/dev/slne/surf/api/minestom/server/visibility/PlayerVisibilityFeature.kt +++ b/surf-api-minestom/surf-api-minestom-server-player-visibility/src/main/kotlin/dev/slne/surf/api/minestom/server/visibility/PlayerVisibilityFeature.kt @@ -160,6 +160,8 @@ class PlayerVisibilityFeature internal constructor( override fun isLimited(player: Player): Boolean = handler(player)?.visibilityLimited == true + override fun manages(player: Player): Boolean = handler(player) != null + companion object { const val ID = "player-visibility" } diff --git a/surf-api-minestom/surf-api-minestom-server-player-visibility/src/main/kotlin/dev/slne/surf/api/minestom/server/visibility/PlayerVisibilityHandler.kt b/surf-api-minestom/surf-api-minestom-server-player-visibility/src/main/kotlin/dev/slne/surf/api/minestom/server/visibility/PlayerVisibilityHandler.kt index 424044a5a..64ac76511 100644 --- a/surf-api-minestom/surf-api-minestom-server-player-visibility/src/main/kotlin/dev/slne/surf/api/minestom/server/visibility/PlayerVisibilityHandler.kt +++ b/surf-api-minestom/surf-api-minestom-server-player-visibility/src/main/kotlin/dev/slne/surf/api/minestom/server/visibility/PlayerVisibilityHandler.kt @@ -1,6 +1,8 @@ package dev.slne.surf.api.minestom.server.visibility +import dev.slne.surf.api.minestom.player.SurfMinestomPlayer import dev.slne.surf.api.minestom.visibility.PlayerVisibility +import dev.slne.surf.api.shared.api.util.InternalSurfApi import net.minestom.server.entity.AutomaticPlayerVisibility import net.minestom.server.entity.Entity import net.minestom.server.entity.Player @@ -19,6 +21,7 @@ import java.util.function.Predicate * updates run on the service tick. The combined rule reads the selection and the applied rule * through volatile fields. */ +@OptIn(InternalSurfApi::class) internal class PlayerVisibilityHandler(private val player: Player) { private class RuleRequest( val predicate: Predicate?, @@ -78,7 +81,8 @@ internal class PlayerVisibilityHandler(private val player: Player) { // Limits this player's packets to its actual viewers instead of every player in range, // most of whom no longer see it once the limit applies. - player.updateViewableRule(ALWAYS_VIEWABLE) + (player as? SurfMinestomPlayer)?.refreshViewableRule() + ?: player.updateViewableRule(ALWAYS_VIEWABLE) } /** @@ -89,7 +93,8 @@ internal class PlayerVisibilityHandler(private val player: Player) { if (!player.isOnline) return player.updateViewerRule(appliedRule) - player.updateViewableRule(null) + (player as? SurfMinestomPlayer)?.refreshViewableRule() + ?: player.updateViewableRule(null) } fun requestRule(predicate: Predicate?) { diff --git a/surf-api-minestom/surf-api-minestom-server-plugins/api/surf-api-minestom-server-plugins.api b/surf-api-minestom/surf-api-minestom-server-plugins/api/surf-api-minestom-server-plugins.api index 33a9a64ab..48ad17f6e 100644 --- a/surf-api-minestom/surf-api-minestom-server-plugins/api/surf-api-minestom-server-plugins.api +++ b/surf-api-minestom/surf-api-minestom-server-plugins/api/surf-api-minestom-server-plugins.api @@ -14,8 +14,10 @@ public final class dev/slne/surf/api/minestom/server/plugins/PluginsFeature$Comp public final class dev/slne/surf/api/minestom/server/plugins/PluginsFeatureBuilder { public final fun getDirectory ()Ljava/nio/file/Path; public final fun getLoadClassPathPlugins ()Z + public final fun getRequireAll ()Z public final fun setDirectory (Ljava/nio/file/Path;)V public final fun setLoadClassPathPlugins (Z)V + public final fun setRequireAll (Z)V } public final class dev/slne/surf/api/minestom/server/plugins/PluginsFeatureKt { diff --git a/surf-api-minestom/surf-api-minestom-server-plugins/src/main/kotlin/dev/slne/surf/api/minestom/server/plugins/PluginsFeature.kt b/surf-api-minestom/surf-api-minestom-server-plugins/src/main/kotlin/dev/slne/surf/api/minestom/server/plugins/PluginsFeature.kt index 9da61cb54..33736aad6 100644 --- a/surf-api-minestom/surf-api-minestom-server-plugins/src/main/kotlin/dev/slne/surf/api/minestom/server/plugins/PluginsFeature.kt +++ b/surf-api-minestom/surf-api-minestom-server-plugins/src/main/kotlin/dev/slne/surf/api/minestom/server/plugins/PluginsFeature.kt @@ -32,6 +32,18 @@ class PluginsFeatureBuilder internal constructor() { * same Gradle project. */ var loadClassPathPlugins: Boolean = true + + /** + * Whether the server fails to start when a plugin cannot be loaded or enabled. + * + * A plugin cannot be loaded when its description cannot be read, it is rejected, fails to be + * created, throws from `onLoad` or requires a plugin that could not be loaded; it cannot be + * enabled when it throws from `onEnable` or requires a plugin that could not be enabled. A jar + * in [directory] without a `minestom-plugin.json` is no plugin and does not count. When + * `false`, the server logs the failure and runs without that plugin and the plugins that depend + * on it. + */ + var requireAll: Boolean = false } /** @@ -42,8 +54,13 @@ class PluginsFeatureBuilder internal constructor() { * Each plugin jar loaded from the `plugins` directory gets its own class loader, which sees the * server, the plugin itself and the plugins it depends on, but no other plugin. * - * Plugins load in dependency order while the server starts, are enabled on its first tick and are - * disabled in reverse order on shutdown. + * Plugins load in dependency order while the server starts, are enabled right after they are loaded, + * before [dev.slne.surf.api.minestom.server.surfMinestomServer] returns and so before the server + * accepts players, and are disabled in reverse order on shutdown. + * + * Each plugin's data directory, `/`, is created before the plugin is constructed. + * A plugin that fails to load or enable is skipped, unless [PluginsFeatureBuilder.requireAll] is + * set. * * The server's own code cannot see into a plugin class loader, so it can only use plugins on the * system class path. The bootstrap of the server jar places the plugins declared in @@ -68,13 +85,18 @@ class PluginsFeature internal constructor( val directory = settings.directory.createDirectories() val serverClassLoader = PluginsFeature::class.java.classLoader - val classPath = if (settings.loadClassPathPlugins) PluginDiscovery.fromClassPath(serverClassLoader) else emptyList() + val discoveryFailures = linkedMapOf() + val classPath = if (settings.loadClassPathPlugins) { + PluginDiscovery.fromClassPath(serverClassLoader, discoveryFailures) + } else { + emptyList() + } val classPathIds = classPath.mapTo(mutableSetOf()) { it.meta.id } val serverPluginJars = serverPluginJars() val candidates = buildList { addAll(classPath) - for (candidate in PluginDiscovery.fromDirectory(directory)) { + for (candidate in PluginDiscovery.fromDirectory(directory, discoveryFailures)) { val jar = candidate.jar?.toAbsolutePath()?.normalize() when { jar !in serverPluginJars -> add(candidate) @@ -89,11 +111,19 @@ class PluginsFeature internal constructor( this.manager = manager PluginManager.install(manager) - manager.load(candidates) + val failures = discoveryFailures + manager.load(candidates) + if (failures.isNotEmpty() && settings.requireAll) { + // Rolls back here, so that no plugin stays loaded however the runtime handles a failing load + disable(api) + throw requireAllFailure("loaded", failures.keys) + } } override fun enable(api: SurfMinestomServer) { - manager?.enable() + val failures = manager?.enable().orEmpty() + if (failures.isNotEmpty() && settings.requireAll) { + throw requireAllFailure("enabled", failures.keys) + } } override fun disable(api: SurfMinestomServer) { @@ -102,6 +132,11 @@ class PluginsFeature internal constructor( PluginManager.install(null) } + private fun requireAllFailure(action: String, ids: Collection) = IllegalStateException( + "Plugins could not be $action: ${ids.joinToString()}. They are required by requireAll, " + + "the reasons are logged above" + ) + private fun serverPluginJars(): Set = System.getProperty(SERVER_PLUGINS_PROPERTY).orEmpty() .split(File.pathSeparator) diff --git a/surf-api-minestom/surf-api-minestom-server-plugins/src/main/kotlin/dev/slne/surf/api/minestom/server/plugins/impl/PluginCandidate.kt b/surf-api-minestom/surf-api-minestom-server-plugins/src/main/kotlin/dev/slne/surf/api/minestom/server/plugins/impl/PluginCandidate.kt index da5b21f64..0f4b6062e 100644 --- a/surf-api-minestom/surf-api-minestom-server-plugins/src/main/kotlin/dev/slne/surf/api/minestom/server/plugins/impl/PluginCandidate.kt +++ b/surf-api-minestom/surf-api-minestom-server-plugins/src/main/kotlin/dev/slne/surf/api/minestom/server/plugins/impl/PluginCandidate.kt @@ -24,6 +24,9 @@ internal data class PluginCandidate( /** * Finds the plugins in a directory and on the system class path. + * + * A plugin that is found but whose description cannot be read is logged and added to the + * `failures` of the call, keyed by its jar or main class, with the reason. */ internal object PluginDiscovery { private val LOGGER = ComponentLogger.logger() @@ -35,16 +38,16 @@ internal object PluginDiscovery { /** * Every jar in [directory] that carries a `minestom-plugin.json`. */ - fun fromDirectory(directory: Path): List { + fun fromDirectory(directory: Path, failures: MutableMap = mutableMapOf()): List { if (!directory.isDirectory()) return emptyList() return directory.listDirectoryEntries() .filter { it.isRegularFile() && it.extension.equals("jar", ignoreCase = true) } .sortedBy { it.fileName.toString() } - .mapNotNull(::fromJar) + .mapNotNull { jar -> fromJar(jar, failures) } } - fun fromJar(jar: Path): PluginCandidate? = try { + fun fromJar(jar: Path, failures: MutableMap = mutableMapOf()): PluginCandidate? = try { JarFile(jar.toFile()).use { file -> val entry = file.getJarEntry(PluginMeta.FILE_NAME) if (entry == null) { @@ -57,24 +60,27 @@ internal object PluginDiscovery { } } catch (failure: Exception) { LOGGER.error("Could not read the plugin {}", jar.fileName, failure) + failures[jar.fileName.toString()] = "its description could not be read: ${describe(failure)}" null } /** * The plugins shaded into the server itself, listed in [PluginMeta.CLASSPATH_INDEX]. */ - fun fromClassPath(classLoader: ClassLoader): List { - val mainClasses = classLoader.getResources(PluginMeta.CLASSPATH_INDEX).toList() + fun fromClassPath(classLoader: ClassLoader, failures: MutableMap = mutableMapOf()): List { + val mainClasses = classLoader.getResources(PluginMeta.CLASSPATH_INDEX).asSequence() .flatMap { url -> url.openStream().use { it.readBytes().decodeToString() }.lines() } .map { line -> line.substringBefore('#').trim() } .filter(String::isNotEmpty) .distinct() + .toList() return mainClasses.mapNotNull { mainClass -> val path = "${PluginMeta.CLASSPATH_DIRECTORY}$mainClass.json" val resource = classLoader.getResource(path) if (resource == null) { - LOGGER.warn("The plugin {} on the class path has no {}", mainClass, path) + LOGGER.error("The plugin {} on the class path has no {}", mainClass, path) + failures[mainClass] = "its description $path is missing" return@mapNotNull null } @@ -83,10 +89,14 @@ internal object PluginDiscovery { PluginCandidate(meta, jar = null) } catch (failure: Exception) { LOGGER.error("Could not read the plugin {} on the class path", mainClass, failure) + failures[mainClass] = "its description could not be read: ${describe(failure)}" null } } } fun parse(content: String): PluginMeta = json.decodeFromString(PluginMeta.serializer(), content) + + private fun describe(failure: Throwable): String = + failure.message?.let { "${failure.javaClass.simpleName}: $it" } ?: failure.javaClass.simpleName } diff --git a/surf-api-minestom/surf-api-minestom-server-plugins/src/main/kotlin/dev/slne/surf/api/minestom/server/plugins/impl/PluginManagerImpl.kt b/surf-api-minestom/surf-api-minestom-server-plugins/src/main/kotlin/dev/slne/surf/api/minestom/server/plugins/impl/PluginManagerImpl.kt index d3ba750fe..625515f37 100644 --- a/surf-api-minestom/surf-api-minestom-server-plugins/src/main/kotlin/dev/slne/surf/api/minestom/server/plugins/impl/PluginManagerImpl.kt +++ b/surf-api-minestom/surf-api-minestom-server-plugins/src/main/kotlin/dev/slne/surf/api/minestom/server/plugins/impl/PluginManagerImpl.kt @@ -12,6 +12,7 @@ import java.nio.file.Path import kotlin.coroutines.AbstractCoroutineContextElement import kotlin.coroutines.CoroutineContext import kotlin.coroutines.EmptyCoroutineContext +import kotlin.io.path.createDirectories import kotlin.io.path.div /** @@ -20,7 +21,8 @@ import kotlin.io.path.div * Lifecycle functions run to completion on the calling thread. A plugin that throws from `onLoad` * is cleaned up and removed, and plugins that require it are not loaded. A plugin that throws from * `onEnable` is disabled and removed, and plugins that require it are disabled without being - * enabled. [plugins] and [plugin] may be called from any thread. + * enabled. [load] and [enable] return the plugins that failed, so that the caller can decide + * whether to continue. [plugins] and [plugin] may be called from any thread. * * While a lifecycle function runs, and in a plugin's coroutine scope, the thread context class * loader is the plugin's class loader. @@ -42,11 +44,16 @@ internal class PluginManagerImpl( /** * Creates every loadable plugin of [candidates] in dependency order and calls its `onLoad`. * Plugins that cannot be loaded are logged. + * + * @return the reason for each plugin that could not be loaded, keyed by plugin ID, including + * rejected candidates, in the order they failed; empty when every plugin loaded */ - fun load(candidates: List) { + fun load(candidates: List): Map { val order = PluginLoadOrder.order(candidates) + val failures = linkedMapOf() order.rejected.forEach { (id, reason) -> LOGGER.error("Not loading the plugin {}, because {}", id, reason) + failures[id] = reason } val failed = mutableSetOf() @@ -58,6 +65,7 @@ internal class PluginManagerImpl( if (failedDependency != null) { LOGGER.error("Not loading the plugin {}, because its dependency {} failed", meta.id, failedDependency.id) failed += meta.id + failures[meta.id] = "its dependency ${failedDependency.id} failed" continue } @@ -66,27 +74,36 @@ internal class PluginManagerImpl( } catch (failure: Throwable) { LOGGER.error("Could not create the plugin {} from {}", meta.id, candidate.source, failure) failed += meta.id + failures[meta.id] = "it could not be created: ${describe(failure)}" continue } LOGGER.info("Loading {} {}", meta.name, meta.version) - if (!runLifecycle(loadedPlugin, "load") { loadedPlugin.plugin.onLoad() }) { + val loadFailure = runLifecycle(loadedPlugin, "load") { loadedPlugin.plugin.onLoad() } + if (loadFailure != null) { cleanUp(loadedPlugin) loaders -= meta.id failed += meta.id + failures[meta.id] = "onLoad failed: ${describe(loadFailure)}" continue } synchronized(loaded) { loaded += loadedPlugin } } + + return failures } /** * Calls `onEnable` of every loaded plugin. A plugin that fails, or whose required dependency * failed, is disabled, which includes calling its `onDisable`. + * + * @return the reason for each plugin that could not be enabled, keyed by plugin ID, in the + * order they failed; empty when every plugin was enabled */ - fun enable() { + fun enable(): Map { val failed = mutableSetOf() + val failures = linkedMapOf() for (loadedPlugin in synchronized(loaded) { loaded.toList() }) { val meta = loadedPlugin.plugin.meta @@ -94,17 +111,22 @@ internal class PluginManagerImpl( if (failedDependency != null) { LOGGER.error("Not enabling the plugin {}, because its dependency {} failed", meta.id, failedDependency.id) failed += meta.id + failures[meta.id] = "its dependency ${failedDependency.id} failed" disable(loadedPlugin) continue } LOGGER.info("Enabling {} {}", meta.name, meta.version) - if (!runLifecycle(loadedPlugin, "enable") { loadedPlugin.plugin.onEnable() }) { + val enableFailure = runLifecycle(loadedPlugin, "enable") { loadedPlugin.plugin.onEnable() } + if (enableFailure != null) { failed += meta.id + failures[meta.id] = "onEnable failed: ${describe(enableFailure)}" disable(loadedPlugin) } } + + return failures } /** @@ -142,6 +164,9 @@ internal class PluginManagerImpl( var scope: CoroutineScope? = null try { + // Created before the plugin, so that it exists while the plugin initializes + val dataDirectory = (directory / meta.id).createDirectories() + // The context must exist during construction so property initializers can use it; for an // `object` that is during class initialization. val plugin = SurfMinestomPlugin.constructing({ plugin -> @@ -161,7 +186,7 @@ internal class PluginManagerImpl( SurfMinestomPlugin.Context( meta = meta, eventNode = eventNode, - dataDirectory = directory / meta.id, + dataDirectory = dataDirectory, logger = logger, scope = pluginScope, ) @@ -217,22 +242,26 @@ internal class PluginManagerImpl( } /** - * Runs a lifecycle function of the plugin and returns whether it completed without throwing. + * Runs a lifecycle function of the plugin and returns what it threw, or `null` when it + * completed. */ private fun runLifecycle( loadedPlugin: LoadedPlugin, phase: String, block: suspend () -> Unit, - ): Boolean = try { + ): Throwable? = try { val context = SurfMinestomPlugin.contextElement(loadedPlugin.plugin) + contextClassLoaderElement(loadedPlugin.classLoader) runBlocking(context) { block() } - true + null } catch (failure: Throwable) { LOGGER.error("Could not {} the plugin {}", phase, loadedPlugin.plugin.meta.id, failure) - false + failure } + private fun describe(failure: Throwable): String = + failure.message?.let { "${failure.javaClass.simpleName}: $it" } ?: failure.javaClass.simpleName + private fun cleanUp(loadedPlugin: LoadedPlugin) { val plugin = loadedPlugin.plugin diff --git a/surf-api-minestom/surf-api-minestom-server-plugins/src/test/kotlin/dev/slne/surf/api/minestom/server/plugins/PluginsFeatureTest.kt b/surf-api-minestom/surf-api-minestom-server-plugins/src/test/kotlin/dev/slne/surf/api/minestom/server/plugins/PluginsFeatureTest.kt index 8630c300f..97dc2b447 100644 --- a/surf-api-minestom/surf-api-minestom-server-plugins/src/test/kotlin/dev/slne/surf/api/minestom/server/plugins/PluginsFeatureTest.kt +++ b/surf-api-minestom/surf-api-minestom-server-plugins/src/test/kotlin/dev/slne/surf/api/minestom/server/plugins/PluginsFeatureTest.kt @@ -6,12 +6,7 @@ import dev.slne.surf.api.minestom.server.plugins.impl.PluginDiscovery import dev.slne.surf.api.minestom.server.surfMinestomServer import net.minestom.testing.Env import net.minestom.testing.EnvTest -import org.junit.jupiter.api.Assertions.assertEquals -import org.junit.jupiter.api.Assertions.assertFalse -import org.junit.jupiter.api.Assertions.assertNotNull -import org.junit.jupiter.api.Assertions.assertNull -import org.junit.jupiter.api.Assertions.assertSame -import org.junit.jupiter.api.Assertions.assertTrue +import org.junit.jupiter.api.Assertions.* import org.junit.jupiter.api.Test import org.junit.jupiter.api.io.TempDir import java.nio.file.Path @@ -40,7 +35,7 @@ class PluginsFeatureTest { assertSame(TestPlugin.instance, plugin) assertEquals("TestPlugin", plugin!!.meta.name) assertEquals(directory.resolve("test-plugin"), plugin.dataDirectory) - assertEquals(listOf("load"), TestPlugin.calls) + assertEquals(listOf("load", "enable"), TestPlugin.calls, "the plugin was not enabled before the start returned") assertTrue(commands.commandExists("test-plugin-command")) assertTrue(plugin.eventNode.parent != null) plugin @@ -48,7 +43,7 @@ class PluginsFeatureTest { server.shutdown() } - assertEquals(listOf("load", "disable"), TestPlugin.calls) + assertEquals(listOf("load", "enable", "disable"), TestPlugin.calls) assertFalse(commands.commandExists("test-plugin-command"), "the plugin's command is unregistered") assertNull(plugin.eventNode.parent, "the plugin's event node is removed") } diff --git a/surf-api-minestom/surf-api-minestom-server-plugins/src/test/kotlin/dev/slne/surf/api/minestom/server/plugins/PluginsRequireAllTest.kt b/surf-api-minestom/surf-api-minestom-server-plugins/src/test/kotlin/dev/slne/surf/api/minestom/server/plugins/PluginsRequireAllTest.kt new file mode 100644 index 000000000..31b4cced1 --- /dev/null +++ b/surf-api-minestom/surf-api-minestom-server-plugins/src/test/kotlin/dev/slne/surf/api/minestom/server/plugins/PluginsRequireAllTest.kt @@ -0,0 +1,233 @@ +package dev.slne.surf.api.minestom.server.plugins + +import dev.slne.surf.api.minestom.plugin.PluginDependency +import dev.slne.surf.api.minestom.plugin.PluginManager +import dev.slne.surf.api.minestom.plugin.PluginMeta +import dev.slne.surf.api.minestom.server.SurfMinestomServer +import dev.slne.surf.api.minestom.server.plugins.impl.PluginCandidate +import dev.slne.surf.api.minestom.server.plugins.impl.PluginDiscovery +import dev.slne.surf.api.minestom.server.plugins.impl.PluginManagerImpl +import net.minestom.server.event.EventNode +import org.junit.jupiter.api.Assertions.* +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.io.TempDir +import java.lang.reflect.Proxy +import java.net.URLClassLoader +import java.nio.file.Path +import java.util.jar.JarEntry +import java.util.jar.JarOutputStream +import kotlin.io.path.* + +class PluginsRequireAllTest { + + /** + * A [SurfMinestomServer] that only provides the event node, which is all the plugins feature + * uses of it. + */ + private fun fakeServer(): SurfMinestomServer { + val eventNode = EventNode.all("test") + return Proxy.newProxyInstance( + SurfMinestomServer::class.java.classLoader, + arrayOf(SurfMinestomServer::class.java), + ) { _, method, _ -> + when (method.name) { + "getEventNode" -> eventNode + else -> throw UnsupportedOperationException(method.name) + } + } as SurfMinestomServer + } + + /** + * Writes a plugin jar that only carries its descriptor; the main class comes from the test class + * path through the parent class loader. + */ + private fun writePlugin(directory: Path, id: String, main: Class<*>, dependencies: String = "") = + writeJar( + directory / "$id.jar", + PluginMeta.FILE_NAME, + """{ "id": "$id", "main": "${main.name}", "version": "1.0", "dependencies": [$dependencies] }""", + ) + + private fun writeJar(jar: Path, entry: String, content: String) { + JarOutputStream(jar.outputStream()).use { out -> + out.putNextEntry(JarEntry(entry)) + out.write(content.toByteArray()) + out.closeEntry() + } + } + + private fun feature(directory: Path, requireAll: Boolean) = PluginsFeature( + PluginsFeatureBuilder().apply { + this.directory = directory + loadClassPathPlugins = false + this.requireAll = requireAll + } + ) + + private fun withFeature(directory: Path, requireAll: Boolean, block: (PluginsFeature, SurfMinestomServer) -> T): T { + val api = fakeServer() + val feature = feature(directory, requireAll) + try { + return block(feature, api) + } finally { + feature.disable(api) + } + } + + @Test + fun `requireAll fails the load when a plugin throws from onLoad`(@TempDir directory: Path) { + RecordingPlugin.events.clear() + writePlugin(directory, "broken", FailingLoadPlugin::class.java) + writePlugin(directory, "healthy", RecordingPlugin::class.java) + + withFeature(directory, requireAll = true) { feature, api -> + val failure = assertThrows(IllegalStateException::class.java) { feature.load(api) } + assertTrue("broken" in failure.message.orEmpty(), failure.message) + assertFalse("healthy" in failure.message.orEmpty(), failure.message) + } + + assertEquals(listOf("load", "disable"), RecordingPlugin.events, "the loaded plugins are rolled back") + assertThrows(IllegalStateException::class.java) { PluginManager.instance } + } + + @Test + fun `requireAll fails the load when a required dependency is missing`(@TempDir directory: Path) { + writePlugin(directory, "orphan", RecordingPlugin::class.java, """{ "id": "absent" }""") + + withFeature(directory, requireAll = true) { feature, api -> + val failure = assertThrows(IllegalStateException::class.java) { feature.load(api) } + assertTrue("orphan" in failure.message.orEmpty(), failure.message) + } + } + + @Test + fun `requireAll fails the load when a required dependency failed`(@TempDir directory: Path) { + writePlugin(directory, "broken", FailingLoadPlugin::class.java) + writePlugin(directory, "dependent", RecordingPlugin::class.java, """{ "id": "broken" }""") + + withFeature(directory, requireAll = true) { feature, api -> + val failure = assertThrows(IllegalStateException::class.java) { feature.load(api) } + assertTrue("broken" in failure.message.orEmpty(), failure.message) + assertTrue("dependent" in failure.message.orEmpty(), failure.message) + } + } + + @Test + fun `requireAll fails the load when a plugin description cannot be read`(@TempDir directory: Path) { + writeJar(directory / "unreadable.jar", PluginMeta.FILE_NAME, """{ "id": """) + writePlugin(directory, "healthy", RecordingPlugin::class.java) + + withFeature(directory, requireAll = true) { feature, api -> + val failure = assertThrows(IllegalStateException::class.java) { feature.load(api) } + assertTrue("unreadable.jar" in failure.message.orEmpty(), failure.message) + assertFalse("healthy" in failure.message.orEmpty(), failure.message) + } + } + + @Test + fun `a jar without a plugin description does not count for requireAll`(@TempDir directory: Path) { + writeJar(directory / "library.jar", "library.txt", "not a plugin") + writePlugin(directory, "healthy", RecordingPlugin::class.java) + + withFeature(directory, requireAll = true) { feature, api -> + feature.load(api) + assertEquals(listOf("healthy"), PluginManager.instance.plugins.map { it.meta.id }) + } + } + + @Test + fun `discovery reports a class path plugin whose description is missing`(@TempDir directory: Path) { + // A directory instead of a jar, whose cached handle would keep the temporary directory on Windows + (directory / PluginMeta.CLASSPATH_INDEX).createParentDirectories().writeText("com.example.MissingPlugin\n") + + val failures = linkedMapOf() + val candidates = URLClassLoader(arrayOf(directory.toUri().toURL()), null).use { loader -> + PluginDiscovery.fromClassPath(loader, failures) + } + + assertEquals(emptyList(), candidates) + assertEquals(setOf("com.example.MissingPlugin"), failures.keys) + } + + @Test + fun `requireAll fails the enable when a plugin throws from onEnable`(@TempDir directory: Path) { + writePlugin(directory, "broken", FailingEnablePlugin::class.java) + writePlugin(directory, "healthy", RecordingPlugin::class.java) + + withFeature(directory, requireAll = true) { feature, api -> + feature.load(api) + + val failure = assertThrows(IllegalStateException::class.java) { feature.enable(api) } + assertTrue("broken" in failure.message.orEmpty(), failure.message) + assertFalse("healthy" in failure.message.orEmpty(), failure.message) + } + } + + @Test + fun `plugins that fail to load or enable are skipped by default`(@TempDir directory: Path) { + RecordingPlugin.events.clear() + writePlugin(directory, "broken-load", FailingLoadPlugin::class.java) + writePlugin(directory, "broken-enable", FailingEnablePlugin::class.java) + writePlugin(directory, "orphan", RecordingPlugin::class.java, """{ "id": "absent" }""") + writePlugin(directory, "healthy", RecordingPlugin::class.java) + + withFeature(directory, requireAll = false) { feature, api -> + feature.load(api) + feature.enable(api) + + val manager = PluginManager.instance + assertEquals(listOf("healthy"), manager.plugins.map { it.meta.id }) + assertEquals(listOf("load", "enable"), RecordingPlugin.events) + } + } + + @Test + fun `the manager reports every plugin that failed with its reason`(@TempDir directory: Path) { + fun candidate(id: String, main: Class<*>, vararg dependencies: String) = PluginCandidate( + PluginMeta( + id, + main.name, + dependencies = dependencies.map { PluginDependency(it) }, + ), + jar = null, + ) + + val manager = PluginManagerImpl(directory, EventNode.all("test"), PluginsRequireAllTest::class.java.classLoader) + try { + val loadFailures = manager.load( + listOf( + candidate("broken-load", FailingLoadPlugin::class.java), + candidate("dependent", RecordingPlugin::class.java, "broken-load"), + candidate("orphan", RecordingPlugin::class.java, "absent"), + candidate("broken-enable", FailingEnablePlugin::class.java), + candidate("healthy", RecordingPlugin::class.java), + ) + ) + + assertEquals(setOf("broken-load", "dependent", "orphan"), loadFailures.keys) + assertTrue("onLoad fails on purpose" in loadFailures.getValue("broken-load")) + assertTrue("broken-load" in loadFailures.getValue("dependent")) + + val enableFailures = manager.enable() + assertEquals(setOf("broken-enable"), enableFailures.keys) + assertTrue("onEnable fails on purpose" in enableFailures.getValue("broken-enable")) + } finally { + manager.disableAll() + } + } + + @Test + fun `the data directory exists before the plugin is constructed`(@TempDir directory: Path) { + DataDirectoryPlugin.constructionSawDirectory = null + DataDirectoryPlugin.loadSawDirectory = null + writePlugin(directory, "data-plugin", DataDirectoryPlugin::class.java) + + withFeature(directory, requireAll = true) { feature, api -> + feature.load(api) + + assertEquals(true, DataDirectoryPlugin.constructionSawDirectory) + assertEquals(true, DataDirectoryPlugin.loadSawDirectory) + assertTrue((directory / "data-plugin").isDirectory()) + } + } +} diff --git a/surf-api-minestom/surf-api-minestom-server-plugins/src/test/kotlin/dev/slne/surf/api/minestom/server/plugins/RequireAllTestPlugins.kt b/surf-api-minestom/surf-api-minestom-server-plugins/src/test/kotlin/dev/slne/surf/api/minestom/server/plugins/RequireAllTestPlugins.kt new file mode 100644 index 000000000..e7d37847d --- /dev/null +++ b/surf-api-minestom/surf-api-minestom-server-plugins/src/test/kotlin/dev/slne/surf/api/minestom/server/plugins/RequireAllTestPlugins.kt @@ -0,0 +1,60 @@ +package dev.slne.surf.api.minestom.server.plugins + +import dev.slne.surf.api.minestom.plugin.SurfMinestomPlugin +import java.nio.file.Files + +/** + * A plugin that records its lifecycle calls and never fails. + */ +class RecordingPlugin : SurfMinestomPlugin() { + override suspend fun onLoad() { + events += "load" + } + + override suspend fun onEnable() { + events += "enable" + } + + override suspend fun onDisable() { + events += "disable" + } + + companion object { + val events = mutableListOf() + } +} + +/** + * A plugin whose `onLoad` throws. + */ +class FailingLoadPlugin : SurfMinestomPlugin() { + override suspend fun onLoad() { + error("onLoad fails on purpose") + } +} + +/** + * A plugin whose `onEnable` throws. + */ +class FailingEnablePlugin : SurfMinestomPlugin() { + override suspend fun onEnable() { + error("onEnable fails on purpose") + } +} + +/** + * A plugin that records whether its data directory exists while it is constructed and loaded. + */ +class DataDirectoryPlugin : SurfMinestomPlugin() { + private val existedOnConstruction = Files.isDirectory(dataDirectory) + + override suspend fun onLoad() { + constructionSawDirectory = existedOnConstruction + loadSawDirectory = Files.isDirectory(dataDirectory) + } + + companion object { + var constructionSawDirectory: Boolean? = null + var loadSawDirectory: Boolean? = null + } +} diff --git a/surf-api-minestom/surf-api-minestom-server/api/surf-api-minestom-server.api b/surf-api-minestom/surf-api-minestom-server/api/surf-api-minestom-server.api index de490a3a4..e4864f304 100644 --- a/surf-api-minestom/surf-api-minestom-server/api/surf-api-minestom-server.api +++ b/surf-api-minestom/surf-api-minestom-server/api/surf-api-minestom-server.api @@ -13,12 +13,17 @@ public final class dev/slne/surf/api/minestom/server/SurfMinestomFeature$Default public static fun load (Ldev/slne/surf/api/minestom/server/SurfMinestomFeature;Ldev/slne/surf/api/minestom/server/SurfMinestomServer;)V } +public abstract interface class dev/slne/surf/api/minestom/server/SurfMinestomPlayerFactory { + public abstract fun create (Lnet/minestom/server/network/player/PlayerConnection;Lnet/minestom/server/network/player/GameProfile;)Ldev/slne/surf/api/minestom/player/SurfMinestomPlayer; +} + public abstract interface class dev/slne/surf/api/minestom/server/SurfMinestomServer { public static final field Companion Ldev/slne/surf/api/minestom/server/SurfMinestomServer$Companion; public abstract fun feature (Ljava/lang/String;)Ldev/slne/surf/api/minestom/server/SurfMinestomFeature; public abstract fun getEventNode ()Lnet/minestom/server/event/EventNode; public abstract fun getFeatures ()Ljava/util/Collection; public abstract fun shutdown ()V + public abstract fun stop ()V } public final class dev/slne/surf/api/minestom/server/SurfMinestomServer$Companion { @@ -34,7 +39,9 @@ public final class dev/slne/surf/api/minestom/server/SurfMinestomServerBuilder { public final fun install (Ldev/slne/surf/api/minestom/server/SurfMinestomFeature;)V public final fun isInstalled (Ljava/lang/String;)Z public final fun listeners ([Ldev/slne/surf/api/minestom/event/EventRegistrar;)V + public final fun onShutdown (Lkotlin/jvm/functions/Function0;)V public final fun permissions (Ldev/slne/surf/api/minestom/permission/PermissionProvider;)V + public final fun playerFactory (Ldev/slne/surf/api/minestom/server/SurfMinestomPlayerFactory;)V public final fun setEventNodeName (Ljava/lang/String;)V public final fun setMaxPlayers (Ljava/lang/Integer;)V public final fun setRecommendedServerProperties (Z)V @@ -256,3 +263,8 @@ public final class dev/slne/surf/api/minestom/server/impl/performance/SkylightSu public fun set ([B)V } +public final class dev/slne/surf/api/minestom/server/impl/player/SurfPlayerProviders { + public static final field INSTANCE Ldev/slne/surf/api/minestom/server/impl/player/SurfPlayerProviders; + public static final fun checkProvider (Lnet/minestom/server/network/PlayerProvider;)V +} + diff --git a/surf-api-minestom/surf-api-minestom-server/src/main/java/dev/slne/surf/api/minestom/server/impl/mixin/SurfMixinAgent.java b/surf-api-minestom/surf-api-minestom-server/src/main/java/dev/slne/surf/api/minestom/server/impl/mixin/SurfMixinAgent.java index b4d4b198e..66589c20e 100644 --- a/surf-api-minestom/surf-api-minestom-server/src/main/java/dev/slne/surf/api/minestom/server/impl/mixin/SurfMixinAgent.java +++ b/surf-api-minestom/surf-api-minestom-server/src/main/java/dev/slne/surf/api/minestom/server/impl/mixin/SurfMixinAgent.java @@ -27,9 +27,11 @@ * system class path, before any other agent or {@code main} runs. The transformer must exist * before the first Minestom class is loaded, because classes loaded earlier are never transformed.

* - *

The mixin configurations come from the {@value #CONFIGS_ATTRIBUTE} manifest attribute, which - * the surf Gradle plugin writes from the installed features and the mixin configurations the - * server registers. Mixin can be installed once per JVM.

+ *

The mixin configurations come from the {@value #CONFIGS_ATTRIBUTE} manifest attribute of the + * server jar, which the surf Gradle plugin writes from the installed features and the mixin + * configurations the server registers. The server jar is the one the bootstrap agent reports in + * {@code surf.minestom.server-jar}, since this class may come from a downloaded library, or else + * the jar this class was loaded from. Mixin can be installed once per JVM.

* *

A required mixin whose target was loaded before the installation fails the installation. A * transformation that fails later makes the JVM load the class untransformed; such classes are @@ -43,6 +45,11 @@ public final class SurfMixinAgent { */ public static final String CONFIGS_ATTRIBUTE = "Surf-Mixin-Configs"; + /** + * The system property {@code SurfMinestomBootstrap} sets to the path of the server jar. + */ + private static final String SERVER_JAR_PROPERTY = "surf.minestom.server-jar"; + private static volatile boolean installed; private static volatile @Nullable Throwable installationFailure; @@ -137,31 +144,42 @@ private static void advancePhases() { } private static List configsFromManifest() { - final @Nullable CodeSource codeSource = SurfMixinAgent.class.getProtectionDomain().getCodeSource(); - if (codeSource == null) { + final @Nullable Path jar = serverJar(); + if (jar == null || !Files.isRegularFile(jar)) { return List.of(); } - try { - final Path jar = Path.of(codeSource.getLocation().toURI()); - if (!Files.isRegularFile(jar)) { + try (JarFile jarFile = new JarFile(jar.toFile())) { + final @Nullable Manifest manifest = jarFile.getManifest(); + final @Nullable String configs = manifest == null ? null : manifest.getMainAttributes().getValue(CONFIGS_ATTRIBUTE); + if (configs == null) { return List.of(); } - try (JarFile jarFile = new JarFile(jar.toFile())) { - final @Nullable Manifest manifest = jarFile.getManifest(); - final @Nullable String configs = manifest == null ? null : manifest.getMainAttributes().getValue(CONFIGS_ATTRIBUTE); - if (configs == null) { - return List.of(); - } - - return Arrays.stream(configs.split(",")) - .map(String::trim) - .filter(config -> !config.isEmpty()) - .toList(); - } - } catch (final IOException | URISyntaxException e) { + return Arrays.stream(configs.split(",")) + .map(String::trim) + .filter(config -> !config.isEmpty()) + .toList(); + } catch (final IOException e) { throw new IllegalStateException("Failed to read the mixin configurations from the server jar", e); } } + + private static @Nullable Path serverJar() { + final @Nullable String reported = System.getProperty(SERVER_JAR_PROPERTY); + if (reported != null) { + return Path.of(reported); + } + + final @Nullable CodeSource codeSource = SurfMixinAgent.class.getProtectionDomain().getCodeSource(); + if (codeSource == null) { + return null; + } + + try { + return Path.of(codeSource.getLocation().toURI()); + } catch (final URISyntaxException e) { + throw new IllegalStateException("Failed to locate the server jar", e); + } + } } diff --git a/surf-api-minestom/surf-api-minestom-server/src/main/java/dev/slne/surf/api/minestom/server/mixin/ConnectionManagerMixin.java b/surf-api-minestom/surf-api-minestom-server/src/main/java/dev/slne/surf/api/minestom/server/mixin/ConnectionManagerMixin.java index 9b11c41f3..29f4884ce 100644 --- a/surf-api-minestom/surf-api-minestom-server/src/main/java/dev/slne/surf/api/minestom/server/mixin/ConnectionManagerMixin.java +++ b/surf-api-minestom/surf-api-minestom-server/src/main/java/dev/slne/surf/api/minestom/server/mixin/ConnectionManagerMixin.java @@ -2,9 +2,12 @@ import dev.slne.surf.api.minestom.server.impl.configuration.ConfigurationPhaseHook; import dev.slne.surf.api.minestom.server.impl.mixin.duck.ConnectionManagerDuck; +import dev.slne.surf.api.minestom.server.impl.player.SurfPlayerProviders; import java.util.Set; import net.minestom.server.entity.Player; import net.minestom.server.network.ConnectionManager; +import net.minestom.server.network.PlayerProvider; +import org.jspecify.annotations.Nullable; import org.spongepowered.asm.mixin.Mixin; import org.spongepowered.asm.mixin.gen.Accessor; import org.spongepowered.asm.mixin.injection.At; @@ -12,12 +15,22 @@ import org.spongepowered.asm.mixin.injection.callback.CallbackInfo; /** - * Redirects {@code doConfiguration} to {@link ConfigurationPhaseHook} and exposes the private - * player sets through {@link ConnectionManagerDuck}. + * Redirects {@code doConfiguration} to {@link ConfigurationPhaseHook}, guards the player provider + * through {@link SurfPlayerProviders} and exposes the private player sets through + * {@link ConnectionManagerDuck}. */ @Mixin(ConnectionManager.class) abstract class ConnectionManagerMixin implements ConnectionManagerDuck { + @Inject( + method = "setPlayerProvider(Lnet/minestom/server/network/PlayerProvider;)V", + at = @At("HEAD"), + remap = false + ) + private void surf$checkPlayerProvider(final @Nullable PlayerProvider playerProvider, final CallbackInfo ci) { + SurfPlayerProviders.checkProvider(playerProvider); + } + @Accessor(value = "configurationPlayers", remap = false) @Override public abstract Set surf$configurationPlayers(); diff --git a/surf-api-minestom/surf-api-minestom-server/src/main/kotlin/dev/slne/surf/api/minestom/server/SurfMinestomServer.kt b/surf-api-minestom/surf-api-minestom-server/src/main/kotlin/dev/slne/surf/api/minestom/server/SurfMinestomServer.kt index 73350e6bd..8ec0e600d 100644 --- a/surf-api-minestom/surf-api-minestom-server/src/main/kotlin/dev/slne/surf/api/minestom/server/SurfMinestomServer.kt +++ b/surf-api-minestom/surf-api-minestom-server/src/main/kotlin/dev/slne/surf/api/minestom/server/SurfMinestomServer.kt @@ -2,11 +2,14 @@ package dev.slne.surf.api.minestom.server import dev.slne.surf.api.minestom.event.EventRegistrar import dev.slne.surf.api.minestom.permission.PermissionProvider +import dev.slne.surf.api.minestom.player.SurfMinestomPlayer import dev.slne.surf.api.minestom.server.configuration.ConfigurationPhaseBuilder import dev.slne.surf.api.minestom.server.impl.SurfMinestomRuntime import net.minestom.server.entity.EntityType import net.minestom.server.event.Event import net.minestom.server.event.EventNode +import net.minestom.server.network.player.GameProfile +import net.minestom.server.network.player.PlayerConnection @DslMarker annotation class SurfMinestomServerDsl @@ -18,9 +21,10 @@ annotation class SurfMinestomServerDsl * `withSignedChat()` or `withLuckPerms()`, so that adding the module as a dependency is enough to * offer the feature. * - * Features are loaded ordered by [loadPriority], ties in installation order, and disabled in - * reverse. A feature that throws in [enable] or [disable] is logged; the other features are not - * affected. + * Features are loaded ordered by [loadPriority], ties in installation order, then enabled in the + * same order, and disabled in reverse. A feature that throws in [load] or [enable] fails + * [surfMinestomServer], which disables the features loaded so far; a feature that throws in + * [disable] is logged and the other features are still disabled. */ interface SurfMinestomFeature { @@ -41,13 +45,20 @@ interface SurfMinestomFeature { fun load(api: SurfMinestomServer) = Unit /** - * Called on the first tick after [surfMinestomServer] started the api. + * Called once every feature is loaded, while [surfMinestomServer] still runs, so before the + * server accepts players. + * + * The server does not tick yet: work scheduled for a tick runs once it is started, and must not + * be waited for here. */ fun enable(api: SurfMinestomServer) = Unit /** * Called when the server shuts down or [SurfMinestomServer.shutdown] is called, for every * loaded feature, including one whose [enable] failed. + * + * When the server stops, Minestom no longer ticks: work scheduled for a tick does not run and + * must not be waited for here. */ fun disable(api: SurfMinestomServer) = Unit } @@ -64,6 +75,8 @@ class SurfMinestomServerBuilder internal constructor() { internal var configurationPhase: ConfigurationPhaseBuilder? = null internal var permissionProvider: PermissionProvider? = null internal val disabledEntityTicks = mutableSetOf() + internal var playerFactory = SurfMinestomPlayerFactory(::SurfMinestomPlayer) + internal val shutdownActions = mutableListOf<() -> Unit>() /** * The name of the event node the surf api registers its listeners on. It is added as a child @@ -159,6 +172,45 @@ class SurfMinestomServerBuilder internal constructor() { val builder = configurationPhase ?: ConfigurationPhaseBuilder().also { configurationPhase = it } builder.block() } + + /** + * Creates every player of the server with [factory], e.g. to use a subclass of + * [SurfMinestomPlayer] with behavior of the server's own. + * + * Players are [SurfMinestomPlayer]s by default. The surf api installs the factory as Minestom's + * player provider; while the api runs, `ConnectionManager#setPlayerProvider` rejects any other + * provider. A provider set through Minestom before the api starts fails the start, except in + * tests (`minestom.inside-test`), where Minestom's test environment sets one for every test. + */ + fun playerFactory(factory: SurfMinestomPlayerFactory) { + playerFactory = factory + } + + /** + * Runs [action] when the server stops, after Minestom stopped and the features and plugins + * are disabled, e.g. to close a database the server's own code opened. + * + * Actions run in registration order on the thread that stops the server; one that throws is + * logged and the others still run. + * + * @see SurfMinestomServer.stop + */ + fun onShutdown(action: () -> Unit) { + shutdownActions += action + } +} + +/** + * Creates the players of a surf server. + * + * @see SurfMinestomServerBuilder.playerFactory + */ +fun interface SurfMinestomPlayerFactory { + + /** + * Creates the player of [connection], called on the player's login thread. + */ + fun create(connection: PlayerConnection, gameProfile: GameProfile): SurfMinestomPlayer } /** @@ -184,10 +236,28 @@ interface SurfMinestomServer { /** * Disables every feature and the surf api itself. * - * Runs automatically when the server shuts down. + * Runs automatically when Minestom stops, see [stop] for stopping the whole server. Does + * nothing once the api is shut down. A call from another thread while the api starts waits for + * the start, which then fails; a call from within the start, e.g. from a feature's `enable`, + * throws [IllegalStateException]. */ fun shutdown() + /** + * Stops the server: stops Minestom, which shuts down the api, then runs the actions registered + * with [SurfMinestomServerBuilder.onShutdown]. + * + * Blocks until the server is stopped. Runs once; a concurrent call waits until the first one + * has finished, and a later call returns right away. The surf api calls it from a JVM shutdown + * hook as well, so that the actions also run when the process is terminated. + * + * Minestom stops the plugins and its connections on the calling thread, which stays blocked + * until they are stopped, so a call from a tick thread, e.g. directly from the executor of a + * command a player ran, throws [IllegalStateException]. Like [shutdown], it cannot be called + * from within the start. + */ + fun stop() + companion object { /** * The running surf api. @@ -215,10 +285,14 @@ interface SurfMinestomServer { * server.start("0.0.0.0", 25565) * ``` * - * The CommandAPI is ready as soon as this returns, so commands can be registered right after it. + * Every feature is loaded and enabled, and with them every plugin, before this returns, so the + * server is ready for players once it is started. The CommandAPI is ready as well, so commands can + * be registered right after it. * * @throws IllegalStateException when the server is not initialized, the api is already running, * or Mixin is not installed (tests excepted) + * @throws Exception whatever a feature threw while loading or enabling; the features loaded so far + * are disabled again */ fun surfMinestomServer(block: SurfMinestomServerBuilder.() -> Unit = {}): SurfMinestomServer = SurfMinestomRuntime.start(SurfMinestomServerBuilder().apply(block)) diff --git a/surf-api-minestom/surf-api-minestom-server/src/main/kotlin/dev/slne/surf/api/minestom/server/impl/SurfMinestomRuntime.kt b/surf-api-minestom/surf-api-minestom-server/src/main/kotlin/dev/slne/surf/api/minestom/server/impl/SurfMinestomRuntime.kt index 27381b56e..bb08e49bf 100644 --- a/surf-api-minestom/surf-api-minestom-server/src/main/kotlin/dev/slne/surf/api/minestom/server/impl/SurfMinestomRuntime.kt +++ b/surf-api-minestom/surf-api-minestom-server/src/main/kotlin/dev/slne/surf/api/minestom/server/impl/SurfMinestomRuntime.kt @@ -19,25 +19,36 @@ import dev.slne.surf.api.minestom.server.impl.mixin.SurfMixinAgent import dev.slne.surf.api.minestom.server.impl.performance.EntityTickFilter import dev.slne.surf.api.minestom.server.impl.player.PlayerEvents import dev.slne.surf.api.minestom.server.impl.player.PlayerFlightListener +import dev.slne.surf.api.minestom.server.impl.player.SurfPlayerProviders import kotlinx.coroutines.runBlocking import net.kyori.adventure.text.logger.slf4j.ComponentLogger import net.minestom.server.MinecraftServer import net.minestom.server.event.Event import net.minestom.server.event.EventNode import net.minestom.server.property.ServerProperties +import net.minestom.server.thread.TickSchedulerThread +import net.minestom.server.thread.TickThread +import java.util.concurrent.CompletableFuture import java.util.concurrent.atomic.AtomicBoolean import java.util.concurrent.atomic.AtomicReference +import java.util.concurrent.locks.ReentrantLock +import kotlin.concurrent.withLock /** * Lifecycle of the surf api on a Minestom server; at most one runtime exists per JVM. * - * [start] runs the load phase synchronously: it installs the surf listeners and services and - * calls `load` on each feature in `loadPriority` order. Features are enabled on the next tick, and - * a feature whose `enable` throws is logged and skipped. [shutdown] is idempotent and calls - * `disable` on every loaded feature in reverse order, including ones that failed to enable. + * [start] loads and enables synchronously: it installs the surf listeners and services, calls + * `load` on each feature in `loadPriority` order and then `enable` in the same order, so every + * feature and plugin is ready before the server accepts players. A failing [start] rolls back what + * it installed with the same teardown as [shutdown] and rethrows the failure. * - * A failing [start] rolls back what it installed with the same teardown as [shutdown] and rethrows - * the failure. A teardown step that throws is logged and does not skip the following steps. + * [shutdown] is idempotent and calls `disable` on every loaded feature in reverse order. A teardown + * step that throws is logged and does not skip the following steps. Startup and teardown hold + * [lifecycleLock]: a shutdown requested by another thread while the api starts waits for the + * startup, which then fails and rolls back. A shutdown from the starting thread itself, e.g. from a + * feature's `enable`, is refused, since the startup would continue on a torn down api. + * + * [stop] stops the whole server once and runs the registered shutdown actions afterwards. */ internal class SurfMinestomRuntime private constructor( private val builder: SurfMinestomServerBuilder, @@ -56,9 +67,28 @@ internal class SurfMinestomRuntime private constructor( private var packetEventsPrepared = false + private val lifecycleLock = ReentrantLock() + + private val shutdownRequested = AtomicBoolean(false) + + @Volatile + private var startingThread: Thread? = null + + private val stopping = AtomicBoolean(false) + + /** + * Completed once [stop] has finished, whether or not a step failed. + */ + private val stopped = CompletableFuture() + + @Volatile + private var stoppingThread: Thread? = null + override fun feature(id: String): SurfMinestomFeature? = builder.features[id] private fun load() { + SurfPlayerProviders.install(builder.playerFactory) + if (builder.recommendedServerProperties) RecommendedServerProperties.apply() preparePacketEvents() runBlocking { @@ -95,32 +125,94 @@ internal class SurfMinestomRuntime private constructor( builder.permissionProvider?.let { provider -> MinestomPermissions.installProvider(provider) } running.set(true) - SchedulerManager.scheduleNextTick(::enable) SchedulerManager.buildShutdownTask(::shutdown) + + enable() } private fun enable() { - if (!running.get()) return - - try { - runBlocking { instance.onEnable() } - } catch (failure: Throwable) { - LOGGER.error("Failed to enable the core listeners and the inventory framework", failure) - } + runBlocking { instance.onEnable() } reportFailedMixins() for (feature in features) { + LOGGER.info("Enabling feature {}", feature.id) + feature.enable(this) + } + } + + override fun shutdown() { + checkNotStarting() + shutdownRequested.set(true) + + lifecycleLock.withLock { + if (!running.compareAndSet(true, false)) return + teardown() + } + } + + override fun stop() { + val thread = Thread.currentThread() + check(thread !is TickThread && thread !is TickSchedulerThread) { + "The server cannot be stopped from the tick thread ${thread.name}, which stop() would " + + "block until the plugins and Minestom are stopped; call stop() from another thread" + } + checkNotStarting() + + if (!stopping.compareAndSet(false, true)) { + if (Thread.currentThread() !== stoppingThread) stopped.join() + return + } + + stoppingThread = Thread.currentThread() + try { + // Minestom runs the shutdown of the api as one of its shutdown tasks, before it closes the + // connections try { - feature.enable(this) + if (MinecraftServer.isStarted()) MinecraftServer.stopCleanly() } catch (failure: Throwable) { - LOGGER.error("Failed to enable feature {}", feature.id, failure) + LOGGER.error("Failed to stop the Minestom server cleanly", failure) } + + // Covers a server that never started, and a Minestom stop whose earlier shutdown tasks + // threw before they reached the api + shutdown() + + for (action in builder.shutdownActions) { + try { + action() + } catch (failure: Throwable) { + LOGGER.error("A shutdown action failed", failure) + } + } + } finally { + stopped.complete(Unit) } } - override fun shutdown() { - if (!running.compareAndSet(true, false)) return - teardown() + private fun checkNotStarting() { + check(Thread.currentThread() !== startingThread) { + "The surf api cannot be shut down while it starts; throw from load or enable to fail the start instead" + } + } + + /** + * Stops the server when the JVM shuts down, so that the plugins, the features and the shutdown + * actions also stop on a signal. Installed once the start succeeded; tests do not get one. + * + * The hook stays registered after [shutdown]: [stop] then still runs the shutdown actions. + */ + private fun installShutdownHook() { + if (ServerProperties.INSIDE_TEST.get()) return + + if (ServerProperties.SHUTDOWN_ON_SIGNAL.get()) { + LOGGER.warn( + "Minestom stops the server from a shutdown hook of its own, which runs concurrently " + + "with the one of the surf api; start the server from the jar built by the surf " + + "Gradle plugin, or set -Dminestom.shutdown-on-signal=false" + ) + } + + Runtime.getRuntime().addShutdownHook(Thread(::stop, "surf-minestom-shutdown")) } /** @@ -160,6 +252,7 @@ internal class SurfMinestomRuntime private constructor( step("uninstall the flight listener") { PlayerFlightListener.uninstall() } step("reset the entity tick filter") { EntityTickFilter.configure(emptySet()) } step("uninstall the player events") { PlayerEvents.uninstall() } + step("reset the player provider") { SurfPlayerProviders.uninstall() } step("remove the event node") { GlobalEventHandler.removeChild(eventNode) } if (packetEventsPrepared) { @@ -229,14 +322,24 @@ internal class SurfMinestomRuntime private constructor( val runtime = SurfMinestomRuntime(builder) check(current.compareAndSet(null, runtime)) { "The surf api is already running" } - try { - runtime.load() - } catch (failure: Throwable) { - runtime.teardown()?.let(failure::addSuppressed) - current.compareAndSet(runtime, null) - throw failure + runtime.lifecycleLock.withLock { + runtime.startingThread = Thread.currentThread() + try { + runtime.load() + + // Requested by another thread, which waits for the lock and then finds the api + // torn down + check(!runtime.shutdownRequested.get()) { "The surf api was shut down while it started" } + } catch (failure: Throwable) { + runtime.teardown()?.let(failure::addSuppressed) + current.compareAndSet(runtime, null) + throw failure + } finally { + runtime.startingThread = null + } } + runtime.installShutdownHook() return runtime } } diff --git a/surf-api-minestom/surf-api-minestom-server/src/main/kotlin/dev/slne/surf/api/minestom/server/impl/player/SurfPlayerProviders.kt b/surf-api-minestom/surf-api-minestom-server/src/main/kotlin/dev/slne/surf/api/minestom/server/impl/player/SurfPlayerProviders.kt new file mode 100644 index 000000000..81874792a --- /dev/null +++ b/surf-api-minestom/surf-api-minestom-server/src/main/kotlin/dev/slne/surf/api/minestom/server/impl/player/SurfPlayerProviders.kt @@ -0,0 +1,83 @@ +package dev.slne.surf.api.minestom.server.impl.player + +import dev.slne.surf.api.minestom.extension.ConnectionManager +import dev.slne.surf.api.minestom.server.SurfMinestomPlayerFactory +import net.minestom.server.entity.Player +import net.minestom.server.network.PlayerProvider +import net.minestom.server.network.player.GameProfile +import net.minestom.server.network.player.PlayerConnection +import net.minestom.server.property.ServerProperties + +/** + * Owns Minestom's player provider while the surf api runs. + * + * Between [install] and [uninstall] the provider creates every player with the factory of the + * server, and `ConnectionManagerMixin` rejects any other provider through [checkProvider]. Outside + * of that window Minestom's setter behaves as usual, but a custom provider that is still installed + * when the api starts fails [install] instead of being replaced silently. In tests + * ([ServerProperties.INSIDE_TEST]) it is replaced, since Minestom's test environment installs a + * provider of its own for every test. Without Mixin nothing is enforced and [install] replaces any + * provider. + */ +object SurfPlayerProviders { + + private var owned = false + private var foreignProviderInstalled = false + + /** + * Installs the provider of [factory] and takes ownership of Minestom's player provider. + * + * @param replaceForeignProvider whether a provider installed through Minestom is replaced instead + * of failing the call + * @throws IllegalStateException when a provider other than the surf one is installed and + * [replaceForeignProvider] is `false` + */ + @Synchronized + internal fun install( + factory: SurfMinestomPlayerFactory, + replaceForeignProvider: Boolean = ServerProperties.INSIDE_TEST.get(), + ) { + check(!foreignProviderInstalled || replaceForeignProvider) { + "ConnectionManager#setPlayerProvider installed a provider before the surf api started; " + + "set the player class with surfMinestomServer { playerFactory { connection, profile -> ... } } instead" + } + + ConnectionManager.setPlayerProvider(SurfPlayerProvider(factory)) + owned = true + } + + /** + * Gives up ownership and restores Minestom's default provider. + */ + @Synchronized + internal fun uninstall() { + if (!owned) return + + owned = false + ConnectionManager.setPlayerProvider(null) + } + + /** + * Entry point of `ConnectionManagerMixin`, called before Minestom installs [provider]. + * + * @throws IllegalStateException when the surf api owns the provider and [provider] is not the + * surf one + */ + @JvmStatic + @Synchronized + fun checkProvider(provider: PlayerProvider?) { + val surf = provider is SurfPlayerProvider + check(!owned || surf) { + "The surf api creates the players of this server; set the player class with " + + "surfMinestomServer { playerFactory { connection, profile -> ... } } instead of " + + "ConnectionManager#setPlayerProvider" + } + + foreignProviderInstalled = provider != null && !surf + } + + private class SurfPlayerProvider(private val factory: SurfMinestomPlayerFactory) : PlayerProvider { + override fun createPlayer(connection: PlayerConnection, gameProfile: GameProfile): Player = + factory.create(connection, gameProfile) + } +} diff --git a/surf-api-minestom/surf-api-minestom-server/src/test/kotlin/dev/slne/surf/api/minestom/server/SurfMinestomServerTest.kt b/surf-api-minestom/surf-api-minestom-server/src/test/kotlin/dev/slne/surf/api/minestom/server/SurfMinestomServerTest.kt index e12a124d1..5adfec682 100644 --- a/surf-api-minestom/surf-api-minestom-server/src/test/kotlin/dev/slne/surf/api/minestom/server/SurfMinestomServerTest.kt +++ b/surf-api-minestom/surf-api-minestom-server/src/test/kotlin/dev/slne/surf/api/minestom/server/SurfMinestomServerTest.kt @@ -6,11 +6,15 @@ import dev.slne.surf.api.minestom.permission.MinestomPermissions import dev.slne.surf.api.minestom.permission.PermissionProvider import dev.slne.surf.api.minestom.server.impl.inventory.framework.MinestomInventoryLoader import net.kyori.adventure.util.TriState +import net.minestom.server.thread.TickThread import net.minestom.testing.Env import net.minestom.testing.EnvTest import org.junit.jupiter.api.Assertions.* import org.junit.jupiter.api.Test import org.junit.jupiter.api.assertThrows +import java.time.Duration +import java.util.concurrent.CompletableFuture +import java.util.concurrent.CountDownLatch @EnvTest class SurfMinestomServerTest { @@ -25,6 +29,10 @@ class SurfMinestomServerTest { calls += "load:$id" } + override fun enable(api: SurfMinestomServer) { + calls += "enable:$id" + } + override fun disable(api: SurfMinestomServer) { calls += "disable:$id" } @@ -38,6 +46,11 @@ class SurfMinestomServerTest { } try { + assertEquals( + listOf("load:first", "load:second", "enable:first", "enable:second"), + calls, + "the features were not enabled before the start returned", + ) assertSame(api, SurfMinestomServer.instance) assertSame(first, api.feature("first")) assertThrows { surfMinestomServer() } @@ -52,7 +65,10 @@ class SurfMinestomServerTest { } assertEquals( - listOf("load:first", "load:second", "disable:second", "disable:first"), + listOf( + "load:first", "load:second", "enable:first", "enable:second", + "disable:second", "disable:first", + ), calls, ) assertThrows { SurfMinestomServer.instance } @@ -132,6 +148,40 @@ class SurfMinestomServerTest { } } + @Test + fun `a failing enable fails the start and disables every loaded feature`(env: Env) { + val calls = mutableListOf() + + val failure = assertThrows { + surfMinestomServer { + install(object : SurfMinestomFeature { + override val id = "first" + override fun enable(api: SurfMinestomServer) { + calls += "enable:first" + } + + override fun disable(api: SurfMinestomServer) { + calls += "disable:first" + } + }) + install(object : SurfMinestomFeature { + override val id = "broken" + override fun enable(api: SurfMinestomServer) { + throw IllegalStateException("broken enable") + } + + override fun disable(api: SurfMinestomServer) { + calls += "disable:broken" + } + }) + } + } + + assertEquals("broken enable", failure.message) + assertEquals(listOf("enable:first", "disable:broken", "disable:first"), calls) + assertThrows { SurfMinestomServer.instance } + } + @Test fun `a failing disable does not keep the api from shutting down`(env: Env) { val calls = mutableListOf() @@ -164,4 +214,137 @@ class SurfMinestomServerTest { restarted.shutdown() } } + + @Test + fun `stop disables the features and then runs every shutdown action once`(env: Env) { + val calls = mutableListOf() + + val api = surfMinestomServer { + install(object : SurfMinestomFeature { + override val id = "feature" + override fun disable(api: SurfMinestomServer) { + calls += "disable" + } + }) + onShutdown { calls += "first action" } + onShutdown { throw IllegalStateException("broken action") } + onShutdown { calls += "last action" } + } + + api.stop() + api.stop() + + assertEquals(listOf("disable", "first action", "last action"), calls) + assertThrows { SurfMinestomServer.instance } + } + + @Test + fun `a shutdown action that stops the server again does not wait for itself`(env: Env) { + val calls = mutableListOf() + + lateinit var api: SurfMinestomServer + api = surfMinestomServer { + onShutdown { + api.stop() + calls += "action" + } + } + + api.stop() + + assertEquals(listOf("action"), calls) + } + + @Test + fun `a concurrent stop waits until the first one has finished`(env: Env) { + val actionRunning = CountDownLatch(1) + val finishAction = CountDownLatch(1) + + val api = surfMinestomServer { + onShutdown { + actionRunning.countDown() + finishAction.await() + } + } + + val first = Thread.ofPlatform().start(api::stop) + actionRunning.await() + val second = Thread.ofPlatform().start(api::stop) + + second.join(Duration.ofMillis(200)) + assertTrue(second.isAlive, "a concurrent stop returned before the first one had finished") + + finishAction.countDown() + first.join() + second.join() + } + + @Test + fun `stopping the server from within the start fails the start`(env: Env) { + val failure = assertThrows { + surfMinestomServer { + install(object : SurfMinestomFeature { + override val id = "stopping" + override fun enable(api: SurfMinestomServer) = api.stop() + }) + } + } + + assertTrue("while it starts" in failure.message.orEmpty(), failure.message) + assertThrows { SurfMinestomServer.instance } + + surfMinestomServer().shutdown() + } + + @Test + fun `a shutdown requested by another thread while the api starts fails the start`(env: Env) { + lateinit var shutdownThread: Thread + + val failure = assertThrows { + surfMinestomServer { + install(object : SurfMinestomFeature { + override val id = "concurrent" + override fun enable(api: SurfMinestomServer) { + shutdownThread = Thread.ofPlatform().start(api::shutdown) + awaitWaiting(shutdownThread) + } + }) + } + } + assertEquals("The surf api was shut down while it started", failure.message) + assertThrows("the failed start left the api running") { SurfMinestomServer.instance } + + shutdownThread.join() + surfMinestomServer().shutdown() + } + + @Test + fun `stop refuses to run on a tick thread`(env: Env) { + val api = surfMinestomServer() + + try { + val failure = CompletableFuture() + object : TickThread("surf-test-tick") { + override fun run() { + failure.complete(runCatching { api.stop() }.exceptionOrNull()) + } + }.start() + + assertInstanceOf(IllegalStateException::class.java, failure.join()) + assertSame(api, SurfMinestomServer.instance, "the refused stop shut the api down") + } finally { + api.shutdown() + } + } + + /** + * Waits until [thread] parks, e.g. on the lifecycle lock of the api. + */ + private fun awaitWaiting(thread: Thread) { + val deadline = System.nanoTime() + Duration.ofSeconds(10).toNanos() + while (thread.state != Thread.State.WAITING) { + check(System.nanoTime() < deadline) { "${thread.name} never started waiting" } + Thread.sleep(1) + } + } } diff --git a/surf-api-minestom/surf-api-minestom-server/src/test/kotlin/dev/slne/surf/api/minestom/server/impl/player/SurfPlayerProvidersTest.kt b/surf-api-minestom/surf-api-minestom-server/src/test/kotlin/dev/slne/surf/api/minestom/server/impl/player/SurfPlayerProvidersTest.kt new file mode 100644 index 000000000..d9da54722 --- /dev/null +++ b/surf-api-minestom/surf-api-minestom-server/src/test/kotlin/dev/slne/surf/api/minestom/server/impl/player/SurfPlayerProvidersTest.kt @@ -0,0 +1,89 @@ +package dev.slne.surf.api.minestom.server.impl.player + +import dev.slne.surf.api.minestom.player.SurfMinestomPlayer +import dev.slne.surf.api.minestom.server.SurfMinestomServerBuilder +import dev.slne.surf.api.minestom.server.surfMinestomServer +import net.minestom.server.coordinate.Pos +import net.minestom.server.entity.Player +import net.minestom.server.network.player.GameProfile +import net.minestom.server.network.player.PlayerConnection +import net.minestom.testing.Env +import net.minestom.testing.EnvTest +import org.junit.jupiter.api.Assertions.assertInstanceOf +import org.junit.jupiter.api.Assertions.assertTrue +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.assertThrows + +/** + * Tests that the surf api creates every player with the factory of the server and that + * `ConnectionManagerMixin` keeps other player providers out while the api runs. + */ +@EnvTest +class SurfPlayerProvidersTest { + + @Test + fun `in tests the api replaces the player provider of the test environment`(env: Env) = withApi { + assertInstanceOf(SurfMinestomPlayer::class.java, env.spawnPlayer()) + } + + @Test + fun `players are created with the factory of the server`(env: Env) = + withApi({ playerFactory(::CustomPlayer) }) { + assertInstanceOf(CustomPlayer::class.java, env.spawnPlayer()) + } + + @Test + fun `Minestom refuses another player provider while the api runs`(env: Env) = withApi { + val connections = env.process().connection() + + val failure = assertThrows { + connections.setPlayerProvider { connection, profile -> Player(connection, profile) } + } + assertTrue("playerFactory" in failure.message.orEmpty(), failure.message) + assertThrows { connections.setPlayerProvider(null) } + + assertInstanceOf(SurfMinestomPlayer::class.java, env.spawnPlayer()) + } + + @Test + fun `outside of tests a player provider installed through Minestom fails the start`(env: Env) { + // The test environment installed a player provider of its own + val failure = assertThrows { + SurfPlayerProviders.install(::SurfMinestomPlayer, replaceForeignProvider = false) + } + + assertTrue("playerFactory" in failure.message.orEmpty(), failure.message) + } + + @Test + fun `Minestom accepts player providers again once the api is shut down`(env: Env) { + val connections = env.process().connection() + withApi {} + + connections.setPlayerProvider(::CustomPlayer) + try { + assertInstanceOf(CustomPlayer::class.java, env.spawnPlayer()) + } finally { + connections.setPlayerProvider(null) + } + } + + private fun withApi( + configure: SurfMinestomServerBuilder.() -> Unit = {}, + block: () -> Unit, + ) { + val api = surfMinestomServer(configure) + try { + block() + } finally { + api.shutdown() + } + } + + private fun Env.spawnPlayer(): Player = createPlayer(createFlatInstance(), Pos.ZERO) + + private class CustomPlayer( + connection: PlayerConnection, + profile: GameProfile, + ) : SurfMinestomPlayer(connection, profile) +} diff --git a/surf-api-minestom/surf-api-minestom/api/surf-api-minestom.api b/surf-api-minestom/surf-api-minestom/api/surf-api-minestom.api index c19a143d0..7616924c5 100644 --- a/surf-api-minestom/surf-api-minestom/api/surf-api-minestom.api +++ b/surf-api-minestom/surf-api-minestom/api/surf-api-minestom.api @@ -3335,6 +3335,15 @@ public final class dev/slne/surf/api/minestom/player/PlayerLimit { public final fun setMaxPlayers (Ljava/lang/Integer;)V } +public class dev/slne/surf/api/minestom/player/SurfMinestomPlayer : net/minestom/server/entity/Player { + public fun (Lnet/minestom/server/network/player/PlayerConnection;Lnet/minestom/server/network/player/GameProfile;)V + public fun deleteMessage (Lnet/kyori/adventure/chat/SignedMessage$Signature;)V + protected fun getAddPlayerToList ()Lnet/minestom/server/network/packet/server/play/PlayerInfoUpdatePacket; + public fun pointers ()Lnet/kyori/adventure/pointer/Pointers; + public final fun refreshViewableRule ()V + public fun setGameMode (Lnet/minestom/server/entity/GameMode;)Z +} + public final class dev/slne/surf/api/minestom/player/event/AsyncPlayerCountEvent : net/minestom/server/event/trait/AsyncEvent, net/minestom/server/event/trait/PlayerEvent { public fun (Lnet/minestom/server/entity/Player;ILjava/lang/Integer;Ldev/slne/surf/api/minestom/player/event/AsyncPlayerCountEvent$Result;Lnet/kyori/adventure/text/Component;)V public final fun allow ()V @@ -3514,12 +3523,14 @@ public final class dev/slne/surf/api/minestom/visibility/PlayerVisibility { public static final field INSTANCE Ldev/slne/surf/api/minestom/visibility/PlayerVisibility; public final fun installProvider (Ldev/slne/surf/api/minestom/visibility/PlayerVisibility$Provider;)V public final fun isLimited (Lnet/minestom/server/entity/Player;)Z + public final fun isManaged (Lnet/minestom/server/entity/Player;)Z public final fun updateViewerRule (Lnet/minestom/server/entity/Player;)V public final fun updateViewerRule (Lnet/minestom/server/entity/Player;Ljava/util/function/Predicate;)V } public abstract interface class dev/slne/surf/api/minestom/visibility/PlayerVisibility$Provider { public abstract fun isLimited (Lnet/minestom/server/entity/Player;)Z + public abstract fun manages (Lnet/minestom/server/entity/Player;)Z public abstract fun refreshViewerRule (Lnet/minestom/server/entity/Player;)Z public abstract fun updateViewerRule (Lnet/minestom/server/entity/Player;Ljava/util/function/Predicate;)Z } diff --git a/surf-api-minestom/surf-api-minestom/src/main/kotlin/dev/slne/surf/api/minestom/player/SurfMinestomPlayer.kt b/surf-api-minestom/surf-api-minestom/src/main/kotlin/dev/slne/surf/api/minestom/player/SurfMinestomPlayer.kt new file mode 100644 index 000000000..b9ee8b207 --- /dev/null +++ b/surf-api-minestom/surf-api-minestom/src/main/kotlin/dev/slne/surf/api/minestom/player/SurfMinestomPlayer.kt @@ -0,0 +1,159 @@ +package dev.slne.surf.api.minestom.player + +import dev.slne.surf.api.minestom.chat.deleteSignedMessage +import dev.slne.surf.api.minestom.chat.signedChatSession +import dev.slne.surf.api.minestom.permission.MinestomPermissions +import dev.slne.surf.api.minestom.visibility.PlayerVisibility +import dev.slne.surf.api.shared.api.util.InternalSurfApi +import net.kyori.adventure.chat.SignedMessage +import net.kyori.adventure.permission.PermissionChecker +import net.kyori.adventure.pointer.Pointers +import net.kyori.adventure.pointer.PointersSupplier +import net.minestom.server.entity.GameMode +import net.minestom.server.entity.Player +import net.minestom.server.entity.PlayerHand +import net.minestom.server.network.packet.server.play.PlayerInfoUpdatePacket +import net.minestom.server.network.player.GameProfile +import net.minestom.server.network.player.PlayerConnection +import net.minestom.server.potion.PotionEffect +import java.util.function.Predicate + +/** + * The player of a surf Minestom server. + * + * Adds to Minestom's [Player]: + * - Adventure's [PermissionChecker] pointer resolves through [MinestomPermissions], which + * `Audience.hasPermission` of surf-api-core reads. + * - Game mode switches complete the way vanilla does: a spectator is hidden from everyone but other + * spectators, leaves its vehicle and stops using its item. + * - The entries of the player-list packet keep the signed chat session, and signed messages are + * deleted through the signed chat of the server. + * + * The surf server creates every player as this class or a subclass, using the factory set in + * `surfMinestomServer { playerFactory { ... } }`. Subclasses must not override the viewer or + * viewable rule methods: the player visibility feature calls them on the player. + */ +open class SurfMinestomPlayer( + playerConnection: PlayerConnection, + gameProfile: GameProfile, +) : Player(playerConnection, gameProfile) { + + private companion object { + private val POINTERS_SUPPLIER = PointersSupplier.builder() + .parent(PLAYER_POINTERS_SUPPLIER) + .resolving(PermissionChecker.POINTER, SurfMinestomPlayer::permissionChecker) + .build() + + private val SPECTATORS_ONLY = Predicate { viewer -> + viewer.gameMode == GameMode.SPECTATOR + } + + private val ALWAYS_VIEWABLE = Predicate { true } + } + + private val permissionChecker = PermissionChecker { permission -> + MinestomPermissions.permissionValue(this, permission) + } + + override fun pointers(): Pointers { + return POINTERS_SUPPLIER.view(this) + } + + override fun setGameMode(gameMode: GameMode): Boolean { + val previousGameMode = getGameMode() + if (!super.setGameMode(gameMode)) return false + + completeGameModeSwitch(previousGameMode) + return true + } + + /** + * Applies the viewable rule that matches the game mode and the visibility limit. + * + * A spectator is seen by spectators only. Any other player gets an always-true rule while the + * player visibility limit manages this player, and Minestom's default rule otherwise. Without + * a rule, Minestom sends the player's movement to every chunk viewer, including the players + * the limit hides; the always-true rule restricts it to the actual viewers. + * + * Called by the player visibility feature when it takes over or releases the player. + */ + @InternalSurfApi + fun refreshViewableRule() { + when { + gameMode == GameMode.SPECTATOR -> updateViewableRule(SPECTATORS_ONLY) + PlayerVisibility.isManaged(this) -> updateViewableRule(ALWAYS_VIEWABLE) + else -> updateViewableRule(null) + } + } + + /** + * Keeps the signed chat session in the entries Minestom sends again, e.g. on a skin change; + * clients drop the session of an entry without one and then reject the player's messages. + */ + override fun getAddPlayerToList(): PlayerInfoUpdatePacket { + val packet = super.getAddPlayerToList() + val session = signedChatSession ?: return packet + + return PlayerInfoUpdatePacket( + packet.actions(), + packet.entries().map { entry -> + PlayerInfoUpdatePacket.Entry( + entry.uuid(), entry.username(), entry.properties(), entry.listed(), + entry.latency(), entry.gameMode(), entry.displayName(), session, + entry.listOrder(), entry.displayHat() + ) + } + ) + } + + override fun deleteMessage(signature: SignedMessage.Signature) { + deleteSignedMessage(signature) + } + + /** + * Completes a switch from [previousGameMode] to the current game mode with the parts of the + * vanilla switch Minestom leaves out. Only a switch into or out of spectator mode has any. + */ + private fun completeGameModeSwitch(previousGameMode: GameMode) { + val isSpectator = gameMode == GameMode.SPECTATOR + val wasSpectator = previousGameMode == GameMode.SPECTATOR + + if (isSpectator == wasSpectator) return + + if (isSpectator) { + vehicle?.removePassenger(this) + stopItemUse() + isInvisible = true + } else { + if (isActive) { + stopSpectating() + } + + isInvisible = hasEffect(PotionEffect.INVISIBILITY) + } + + refreshViewableRule() + refreshSpectatorRules() + } + + /** + * Re-runs the viewable rule of every other spectator, which decides whether this player may + * see them. + */ + private fun refreshSpectatorRules() { + val instance = instance ?: return + + for (player in instance.players) { + if (player !== this && player.gameMode == GameMode.SPECTATOR) { + player.updateViewableRule() + } + } + } + + private fun stopItemUse() { + val hand = itemUseHand ?: return + + refreshActiveHand(false, hand == PlayerHand.OFF, false) + clearItemUse() + } +} diff --git a/surf-api-minestom/surf-api-minestom/src/main/kotlin/dev/slne/surf/api/minestom/plugin/SurfMinestomPlugin.kt b/surf-api-minestom/surf-api-minestom/src/main/kotlin/dev/slne/surf/api/minestom/plugin/SurfMinestomPlugin.kt index 8b23c7aae..a8f3dce48 100644 --- a/surf-api-minestom/surf-api-minestom/src/main/kotlin/dev/slne/surf/api/minestom/plugin/SurfMinestomPlugin.kt +++ b/surf-api-minestom/surf-api-minestom/src/main/kotlin/dev/slne/surf/api/minestom/plugin/SurfMinestomPlugin.kt @@ -28,10 +28,11 @@ import java.nio.file.Path * one of the lifecycle functions runs, or from within [scope], belong to the plugin. They are * removed or, for coroutines, cancelled once the plugin is disabled. * - * The lifecycle functions run blocking on the server thread: `onLoad` before the server accepts - * players, `onEnable` on the first tick and `onDisable` during shutdown. They must not wait for - * work dispatched to `MinestomDispatchers.Main` or to an entity scheduler, because that work only - * runs once the blocked thread returns to the tick loop. Such work is launched in [scope] instead. + * The lifecycle functions run blocking: `onLoad` and then `onEnable` while the server starts, + * before it accepts players, and `onDisable` during shutdown. They must not wait for work + * dispatched to `MinestomDispatchers.Main` or to an entity scheduler, because that work only runs + * on a tick: the server does not tick yet while it starts and no longer ticks while it stops. Such + * work is launched in [scope] instead. */ abstract class SurfMinestomPlugin { // Taken in the constructor so that property initializers of the plugin, including those of an @@ -69,7 +70,7 @@ abstract class SurfMinestomPlugin { open suspend fun onLoad() = Unit /** - * Called on the first tick after the server started. + * Called once every plugin is loaded, while the server starts, before it accepts players. */ open suspend fun onEnable() = Unit diff --git a/surf-api-minestom/surf-api-minestom/src/main/kotlin/dev/slne/surf/api/minestom/visibility/PlayerVisibility.kt b/surf-api-minestom/surf-api-minestom/src/main/kotlin/dev/slne/surf/api/minestom/visibility/PlayerVisibility.kt index cf7251504..f78dfcca1 100644 --- a/surf-api-minestom/surf-api-minestom/src/main/kotlin/dev/slne/surf/api/minestom/visibility/PlayerVisibility.kt +++ b/surf-api-minestom/surf-api-minestom/src/main/kotlin/dev/slne/surf/api/minestom/visibility/PlayerVisibility.kt @@ -42,6 +42,11 @@ object PlayerVisibility { */ fun isLimited(player: Player): Boolean = provider?.isLimited(player) == true + /** + * Whether the server's visibility limit owns the viewer and viewable rules of [player]. + */ + fun isManaged(player: Player): Boolean = provider?.manages(player) == true + /** * Implemented by the server when it limits the visible players. */ @@ -58,6 +63,11 @@ object PlayerVisibility { fun refreshViewerRule(player: Player): Boolean fun isLimited(player: Player): Boolean + + /** + * Whether this provider owns the viewer and viewable rules of [player]. + */ + fun manages(player: Player): Boolean } @InternalSurfApi diff --git a/surf-api-minestom/surf-api-minestom/src/test/kotlin/dev/slne/surf/api/minestom/player/SurfMinestomPlayerTest.kt b/surf-api-minestom/surf-api-minestom/src/test/kotlin/dev/slne/surf/api/minestom/player/SurfMinestomPlayerTest.kt new file mode 100644 index 000000000..6dc2df5e0 --- /dev/null +++ b/surf-api-minestom/surf-api-minestom/src/test/kotlin/dev/slne/surf/api/minestom/player/SurfMinestomPlayerTest.kt @@ -0,0 +1,274 @@ +package dev.slne.surf.api.minestom.player + +import dev.slne.surf.api.minestom.chat.ChatSessionHolder +import dev.slne.surf.api.minestom.chat.RemoteChatSender +import dev.slne.surf.api.minestom.chat.RemoteSignedMessage +import dev.slne.surf.api.minestom.permission.MinestomPermissions +import dev.slne.surf.api.minestom.permission.PermissionProvider +import dev.slne.surf.api.shared.api.util.InternalSurfApi +import net.kyori.adventure.chat.SignedMessage +import net.kyori.adventure.permission.PermissionChecker +import net.kyori.adventure.text.Component +import net.kyori.adventure.util.TriState +import net.minestom.server.coordinate.Pos +import net.minestom.server.crypto.ChatSession +import net.minestom.server.crypto.PlayerPublicKey +import net.minestom.server.entity.GameMode +import net.minestom.server.entity.Player +import net.minestom.server.instance.Instance +import net.minestom.server.network.packet.server.play.DestroyEntitiesPacket +import net.minestom.server.network.packet.server.play.PlayerInfoUpdatePacket +import net.minestom.server.network.player.GameProfile +import net.minestom.server.network.player.PlayerConnection +import net.minestom.testing.Env +import net.minestom.testing.EnvTest +import net.minestom.testing.TestConnection +import org.junit.jupiter.api.Assertions.* +import org.junit.jupiter.api.Test +import java.security.KeyPairGenerator +import java.time.Instant +import java.util.* +import java.util.concurrent.CompletableFuture + +@OptIn(InternalSurfApi::class) +@EnvTest +class SurfMinestomPlayerTest { + + @Test + fun `a spectator is taken off every client that is not one`(env: Env) = + withPlayerPair(env) { spectator, other, otherConnection -> + val destroys = otherConnection.trackIncoming(DestroyEntitiesPacket::class.java) + + spectator.switchGameMode(GameMode.SPECTATOR) + + assertFalse(other in spectator.viewers, "a non-spectator was still shown the spectator") + assertTrue(spectator in other.viewers, "the spectator stopped seeing a non-spectator") + assertTrue(spectator.isInvisible, "the spectator was not marked invisible") + assertEquals( + listOf(listOf(spectator.entityId)), + destroys.collect().map(DestroyEntitiesPacket::entityIds), + "the spectator was not removed from the other client", + ) + } + + @Test + fun `spectators keep seeing each other`(env: Env) = + withPlayerPair(env) { spectator, other, _ -> + spectator.switchGameMode(GameMode.SPECTATOR) + other.switchGameMode(GameMode.SPECTATOR) + + assertTrue(other in spectator.viewers, "a spectator was not shown another spectator") + assertTrue(spectator in other.viewers, "a spectator was not shown another spectator") + } + + @Test + fun `leaving spectator shows the player to everyone again`(env: Env) = + withPlayerPair(env) { spectator, other, _ -> + spectator.switchGameMode(GameMode.SPECTATOR) + spectator.switchGameMode(GameMode.ADVENTURE) + + assertTrue(other in spectator.viewers, "the player stayed hidden after spectator") + assertFalse(spectator.isInvisible, "the player stayed invisible after spectator") + } + + @Test + fun `leaving spectator hides the spectators that were visible`(env: Env) = + withPlayerPair(env) { leaving, staying, _ -> + leaving.switchGameMode(GameMode.SPECTATOR) + staying.switchGameMode(GameMode.SPECTATOR) + leaving.switchGameMode(GameMode.ADVENTURE) + + assertFalse(leaving in staying.viewers, "a spectator stayed shown to a non-spectator") + assertTrue(staying in leaving.viewers, "the remaining spectator lost the player") + } + + @Test + fun `a switch between the other modes leaves visibility alone`(env: Env) = + withPlayerPair(env) { player, other, _ -> + player.switchGameMode(GameMode.CREATIVE) + + assertTrue(other in player.viewers, "an ordinary mode switch hid the player") + assertFalse(player.isInvisible, "an ordinary mode switch made the player invisible") + } + + @Test + fun `resolves Adventure's permission checker through the surf api`(env: Env) { + MinestomPermissions.installProvider(PermissionProvider { _, permission -> + if (permission == GRANTED) TriState.TRUE else TriState.NOT_SET + }) + + try { + withPlayerPair(env) { player, _, _ -> + assertInstanceOf(SurfMinestomPlayer::class.java, player) + + val checker = player.get(PermissionChecker.POINTER).orElseThrow() + assertEquals(TriState.TRUE, checker.value(GRANTED)) + assertEquals(TriState.NOT_SET, checker.value("surf.test.other")) + } + } finally { + MinestomPermissions.installProvider(null) + } + } + + @Test + fun `keeps the signed chat session in every player list entry`(env: Env) { + val session = chatSession() + withExposingPlayerPair(env) { player, _ -> + assertNull( + player.addPlayerToList().entries().single().chatSession(), + "the entry carried a session without signed chat", + ) + + ChatSessionHolder.installProvider(SessionProvider(player, session)) + try { + val packet = player.addPlayerToList() + + assertEquals( + listOf(session), + packet.entries().map { entry -> entry.chatSession() }, + "the entry lost the chat session", + ) + } finally { + ChatSessionHolder.installProvider(null) + } + } + } + + private fun Player.switchGameMode(gameMode: GameMode) { + assertTrue(setGameMode(gameMode), "the game mode change was refused") + } + + private fun withPlayerPair( + env: Env, + block: (Player, Player, TestConnection) -> Unit, + ) = withPlayers(env, { connection, profile -> SurfMinestomPlayer(connection, profile) }, block) + + private fun withExposingPlayerPair( + env: Env, + block: (ExposingPlayer, TestConnection) -> Unit, + ) = withPlayers( + env, + { connection, profile -> ExposingPlayer(connection, profile) }, + ) { first, _, secondConnection -> block(first as ExposingPlayer, secondConnection) } + + private fun withPlayers( + env: Env, + factory: (PlayerConnection, GameProfile) -> Player, + block: (Player, Player, TestConnection) -> Unit, + ) { + val connectionManager = env.process().connection() + connectionManager.setPlayerProvider { connection, profile -> factory(connection, profile) } + + val instance = env.createFlatInstance() + val players = mutableListOf() + try { + instance.loadChunk(0, 0).join() + + val (first, _) = connect(env, instance, "First").also { (player, _) -> players += player } + val (second, secondConnection) = connect(env, instance, "Second").also { (player, _) -> players += player } + + awaitSpawn(env, first, second) + assertTrue(second in first.viewers, "the players did not see each other to begin with") + assertTrue(first in second.viewers, "the players did not see each other to begin with") + + block(first, second, secondConnection) + } finally { + players.forEach(Player::remove) + connectionManager.setPlayerProvider(null) + env.destroyInstance(instance) + } + } + + /** Waits for the spawn, which the harness completes off the test thread. */ + private fun awaitSpawn(env: Env, vararg players: Player) { + repeat(SPAWN_ATTEMPTS) { + if (players.all { player -> player.instance != null }) return + env.tick() + Thread.sleep(SPAWN_POLL_MILLIS) + } + + fail("the players never spawned") + } + + private fun connect( + env: Env, + instance: Instance, + username: String, + ): Pair { + val connection = env.createConnection(GameProfile(UUID.randomUUID(), username)) + val player = onVirtualThread { connection.connect(instance, Pos(0.0, 42.0, 0.0)) } + return player to connection + } + + /** Logging a player in asserts that it runs on a virtual thread. */ + private fun onVirtualThread(block: () -> T): T { + val result = CompletableFuture() + + Thread.startVirtualThread { + runCatching(block) + .onSuccess(result::complete) + .onFailure(result::completeExceptionally) + } + + return result.join() + } + + private fun chatSession(): ChatSession { + val key = KeyPairGenerator.getInstance("RSA").generateKeyPair().public + val publicKey = PlayerPublicKey(Instant.now().plusSeconds(3600), key, ByteArray(0)) + return ChatSession(UUID.randomUUID(), publicKey) + } + + private class ExposingPlayer( + connection: PlayerConnection, + profile: GameProfile, + ) : SurfMinestomPlayer(connection, profile) { + fun addPlayerToList(): PlayerInfoUpdatePacket = getAddPlayerToList() + } + + private class SessionProvider( + private val player: Player, + private val session: ChatSession, + ) : ChatSessionHolder.Provider { + private val holder = object : ChatSessionHolder { + override val player: Player get() = this@SessionProvider.player + override val chatSession: ChatSession get() = session + + override fun sendSignedMessage( + message: SignedMessage, + boundName: Component, + unsignedContent: Component?, + ) = unsupported() + + override fun captureSignedMessage( + message: SignedMessage, + unsignedContent: Component?, + ): RemoteSignedMessage? = unsupported() + + override fun sendRemoteSignedMessage( + sender: RemoteChatSender, + message: RemoteSignedMessage, + boundName: Component, + ) = unsupported() + + override fun deleteMessage(signature: SignedMessage.Signature) = unsupported() + } + + override fun fromPlayer(player: Player): ChatSessionHolder? = + holder.takeIf { player === this.player } + + override fun fromUuid(uuid: UUID): ChatSessionHolder? = + holder.takeIf { uuid == player.uuid } + + override fun deleteMessage(player: Player, signature: SignedMessage.Signature) = + unsupported() + + private fun unsupported(): Nothing = error("not needed by the test") + } + + private companion object { + const val GRANTED = "surf.test.granted" + const val SPAWN_ATTEMPTS = 200 + const val SPAWN_POLL_MILLIS = 5L + } +}