Add and implement BukkitScheduler getAsyncExecutor#8595
Conversation
| public class CraftAsyncScheduler extends CraftScheduler { | ||
|
|
||
| - private final ThreadPoolExecutor executor = new ThreadPoolExecutor( | ||
| + final ThreadPoolExecutor executor = new ThreadPoolExecutor( |
There was a problem hiding this comment.
AT, make sure to add it to the patch header
There was a problem hiding this comment.
What does this acronym mean?
There was a problem hiding this comment.
Access transformer, see the paper contributing guide.
| + private void waitForAsyncTasksToShutdown(boolean reload) { | ||
| + | ||
| + // Wait for plugins to close their threads | ||
| int pollCount = 0; | ||
|
|
||
| - // Wait for at most 5 seconds for plugins to close their threads | ||
| - while (pollCount < 10*5 && getScheduler().getActiveWorkers().size() > 0) { | ||
| + // During reload, wait for at most 2.5 seconds, in shutdown, 5 seconds | ||
| + long sleepTime = (reload) ? 50 : 100; | ||
| + | ||
| + while (pollCount < 50 && !getScheduler().isAllFinished()) { | ||
| try { | ||
| - Thread.sleep(100); | ||
| + Thread.sleep(sleepTime); | ||
| } catch (InterruptedException e) {} | ||
| pollCount++; | ||
| } | ||
|
|
||
| - List<BukkitWorker> overdueWorkers = getScheduler().getActiveWorkers(); | ||
| + List<BukkitWorker> overdueWorkers = this.getScheduler().getActiveWorkers(); | ||
| for (BukkitWorker worker : overdueWorkers) { | ||
| Plugin plugin = worker.getOwner(); | ||
| - String author = "<NoAuthorGiven>"; | ||
| - if (plugin.getDescription().getAuthors().size() > 0) { | ||
| - author = plugin.getDescription().getAuthors().get(0); | ||
| + warnNotShuttingDownAsyncTasks(plugin, reload); | ||
| + if (reload && console.isDebugging()) { | ||
| + io.papermc.paper.util.TraceUtil.dumpTraceForThread(worker.getThread(), "still running"); | ||
| } | ||
| - getLogger().log(Level.SEVERE, String.format( | ||
| - "Nag author: '%s' of '%s' about the following: %s", | ||
| - author, | ||
| - plugin.getDescription().getName(), | ||
| - "This plugin is not properly shutting down its async tasks when it is being shut down. This task may throw errors during the final shutdown logs and might not complete before process dies." | ||
| - )); | ||
| } | ||
| + for (Plugin plugin : getScheduler().getUnfinishedFromExecutors()) { | ||
| + warnNotShuttingDownAsyncTasks(plugin, reload); | ||
| + } | ||
| + } | ||
| + | ||
| + private void warnNotShuttingDownAsyncTasks(Plugin plugin, boolean reload) { | ||
| + String message; | ||
| + if (reload) { | ||
| + message = "This plugin is not properly shutting down its async tasks when it is being reloaded. " | ||
| + + "This may cause conflicts with the newly loaded version of the plugin"; | ||
| + } else { | ||
| + message = "This plugin is not properly shutting down its async tasks when it is being shut down. " | ||
| + + "This task may throw errors during the final shutdown logs and might not complete before process dies."; | ||
| + } | ||
| + getLogger().log(Level.SEVERE, String.format( | ||
| + "Nag author(s): '%s' of '%s' about the following: %s", | ||
| + plugin.getDescription().getAuthors(), | ||
| + plugin.getDescription().getFullName(), | ||
| + message | ||
| + )); | ||
| + } | ||
| + | ||
| + public void waitForAsyncTasksShutdown() { | ||
| + waitForAsyncTasksToShutdown(false); | ||
| } | ||
| // Paper end |
There was a problem hiding this comment.
Looks like there's a bit of conflict with a pre-existing paper patch. Can you please merge some of these changes into the already existing Wait for Async Tasks during shutdown patch?
This should hopefully pretty up the diff here.
| } | ||
|
|
||
| - private int nextId() { | ||
| + int nextId() { // Paper - private -> package private |
|
|
||
| // Paper start | ||
| - private final CraftScheduler asyncScheduler; | ||
| + final CraftAsyncScheduler asyncScheduler; |
There was a problem hiding this comment.
Don't change the type up here, instead just cast it when needed.
There was a problem hiding this comment.
The field is also made package-private, which fact is used in PluginExecutor.
| throw new UnsupportedOperationException("Use BukkitRunnable#runTaskTimerAsynchronously(Plugin, long, long)"); | ||
| } | ||
|
|
||
| - // Paper start - add getMainThreadExecutor |
There was a problem hiding this comment.
If you're going to edit paper comments edit them in the patch they came from
| public BukkitTask runTaskTimerAsynchronously(@NotNull Plugin plugin, @NotNull BukkitRunnable task, long delay, long period) throws IllegalArgumentException; | ||
|
|
||
| - // Paper start - add getMainThreadExecutor | ||
| + // Paper start |
There was a problem hiding this comment.
see below about paper comments
|
There are in fact several existing patches whose changes this PR interacts with:
They leave me many choices. I am able to merge them all into one patch, which seems simplest. Alternatively I might edit the existing patches individually, and manipulate patch ordering, so as to produce minimal difference in the latest patch, 0949-Implement-CraftScheduler-getAsyncExecutor-API. There are various combinations of these operations. What is preferred? Additionally, there is of course 185-Improved-Async-Task-Scheduler, on which many other changes are based. |
|
It's mostly that you're overlapping a lot with Wait-for-Async-Tasks-during-shutdown here, mostly editing a method that it adds itself. So, at least the majority of that diff can be shifted to that patch. So at least for now, try to keep the patch noise low for PR viewability. |
kennytv
added
the
pre-softspoon: never rebased
5 participants
This is a second-try / recreation of #4064 . In this PR I renamed the method to
getAsyncExecutor, for symmetry withgetMainThreadExecutor. These methods work nicely in tandem. Their use is particularly relevant when working with CompletableFuture.One could argue they are not strictly counterparts and therefore should be named differently. The existing method
getMainThreadExecutorprovides pure convenience and nothing else. In contrast,getAsyncExecutoradds a new mechanism for using the common thread pool that circumvents traditional tasks; it does not merely redirect calls torunTaskAsynchronously. However, I do not find this argument sufficient justification for creating asymmetrical method names considering their most obvious use-case -- interoperation with CompletableFuture and other task-chaining APIs -- will be the same.