Skip to content

Add BukkitScheduler#createAsyncExecutor API#4064

Closed
A248 wants to merge 1 commit into
PaperMC:masterfrom
A248:create-async-executor
Closed

Add BukkitScheduler#createAsyncExecutor API#4064
A248 wants to merge 1 commit into
PaperMC:masterfrom
A248:create-async-executor

Conversation

@A248
Copy link
Copy Markdown
Contributor

@A248 A248 commented Aug 5, 2020

Currently, the CraftScheduler will commence asynchronous tasks every tick, per CraftAsyncScheduler#mainThreadHeartbeat. However, if the main thread is blocked, async tasks will not begin.

A common use case involves awaiting the result of an asynchronous computation on the main thread. Using the current API in the BukkitScheduler, this might cause a deadlock, since initiation of async tasks requires the main thread to keep beating.

To solve this, I added the BukkitScheduler#createAsyncExecutor API. This method returns an Executor not coupled to the server tick loop. If the main thread is blocked, it will still be able to submit tasks, using the server's cached thread pool.

Why not tell people experiencing this issue to create their own ExecutorService?
That's completely possible, and I do not want to say it is a bad idea. For many cases, an own thread pool would be preferable. However, doing so negates the advantage of the scheduler's common thread pool, which reuses Craft Scheduler Threads as they become available. Also, creating an ExecutorService for a small task is overkill when there is one available.

Error Reporting
I followed the same approach the scheduler normally uses for CraftAsyncTasks - catch-all and print the exception. However, the log message is slightly different to indicate the task was created through such an Executor.

@A248 A248 force-pushed the create-async-executor branch from dfee1f9 to 8971625 Compare August 8, 2020 15:20
Copy link
Copy Markdown
Contributor

@Proximyst Proximyst left a comment

Choose a reason for hiding this comment

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

Some paper comments missing, otherwise LGTM.

Comment thread Spigot-Server-Patches/0551-Implement-CraftScheduler-createAsyncExecutor-API.patch Outdated
Comment thread Spigot-Server-Patches/0551-Implement-CraftScheduler-createAsyncExecutor-API.patch Outdated
Comment thread Spigot-Server-Patches/0551-Implement-CraftScheduler-createAsyncExecutor-API.patch Outdated
@A248 A248 force-pushed the create-async-executor branch from 8971625 to deda936 Compare August 21, 2020 22:14
@A248 A248 requested a review from a team as a code owner August 21, 2020 22:14
Proximyst
Proximyst previously approved these changes Aug 21, 2020
@Proximyst Proximyst added status: rebase required type: feature Request for a new Feature. labels Aug 24, 2020
@A248 A248 force-pushed the create-async-executor branch 2 times, most recently from cdf6936 to 326eb91 Compare October 5, 2020 17:14
Copy link
Copy Markdown
Member

@aikar aikar left a comment

Choose a reason for hiding this comment

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

the scheduler needs to keep track of these when created, and when a plugin is disabled, shut down the executor so that tasks may flush and try to gracefully shutdown and pause the disable event.

But it should use same timeout that current async tasks do where it gives them time to stop but gives up and will shutdown anyways if not.

.shutdown() should be called before the current async tasks waiting code, and the "wait for executors" should factor in all time spent async tasks (ie if we give 30s and async tasks used 20s of it, don't blindly wait for another 30s, it should be 30s total for both combined)

30s is off memory on what it actually is, and is purely for example purposes.
just this waiting shouldn't add any additional time.

@A248
Copy link
Copy Markdown
Contributor Author

A248 commented Oct 6, 2020

I took a slightly different approach which preserves the existing behaviour (present in normal async tasks) of allowing, but strongly warning against, incomplete tasks on reload and shutdown. This I found was slightly easier to implement than calling shutdown(), getting the incomplete tasks, and having to recreate the ExecutorService over reloads.

@Proximyst Proximyst requested a review from aikar November 1, 2020 19:48
* Adds the ability to create an Executor using the BukkitScheduler's cached thread pool which is not coupled to the main thread heartbeat.
* Tracks Runnables from PluginExecutors to ensure clean reload and shutdown
@A248 A248 force-pushed the create-async-executor branch from bd01075 to 46c016a Compare November 2, 2020 14:02
@stale
Copy link
Copy Markdown

stale Bot commented Jan 1, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@A248
Copy link
Copy Markdown
Contributor Author

A248 commented Jan 1, 2021

Not stale, don't close.

This PR is ready for review in terms of code changes. I will rebase it when I get the chance.

@stale stale Bot removed the resolution: stale label Jan 1, 2021
@A248 A248 requested a review from a team as a code owner June 20, 2021 12:00
@Owen1212055
Copy link
Copy Markdown
Member

Closing PR, as it seems that this has gone inactive.

Seems like you did comment that you'll rebase it whenever so feel free to rebase and leave a comment and we can reopen it for you. 😄

A248 added a commit to A248/Paper that referenced this pull request Nov 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants