WiIIiam278 / PAPIProxyBridge

A bridge library plugin for using PlaceholderAPI on proxy servers
https://william278.net/project/papiproxybridge
Apache License 2.0
37 stars 11 forks source link

[question] Why use CompletableFuture.completedFuture in BukkitPAPIProxyBridge? #19

Closed Andre601 closed 1 year ago

Andre601 commented 1 year ago

This was pointed out by someone else, but in your createRequest are you using CompletableFuture.completedFuture. Why?

This is rather pointless to do, as supplyAsync is usually the better aproach for a truly async behaviour here. completedFuture does nothing beneficial here as it returns a CompletableFuture that has been completed already, meaning that this is technically a sync method rather than async.

I would love to know the reasoning behind the usage of this particular method over supplyAsync

WiIIiam278 commented 1 year ago

Hi there,

Since on the bukkit side that logic doesn't need executing asynchronously (since no network messaging is needed here), I return an already-completed future. There's one primary reason for this: Mixing running logic on the ForkJoinPool (which is what supplyAsync does) and platform-specific managed asynchronous tasks very easily leads to exhaustion of the previously mentioned pool, particularly on methods that get called a lot -- which can lead to all sorts of unintended behaviour as the JVM isn't able to allocate threads between the managed pool of tasks. It also immediately bricks other plugins relying on that pool until the server is restarted (some of which are mine; I'm moving away from relying on this feature as a result)

The reason I implement that method on the bukkit-side and that you can even create requests on there to begin with is purely to support multi-platform projects where you just want to depend on one API to do all placeholder handling. Since it returns an already-completed future, thenRun should run on the same thread as the one that called the createRequst method. I do recognise though, that in setups where the expectation is that createRequest runs asynchronously, an expection is being broken on the Bukkit (and fabric where this is also the case) platforms.

I hope this answers your question -- or if perhaps there's some assumptions/bits I'm mistaken on, do feel free to clarify as of course if there's room for improvement I'm keen to strike on that.

Andre601 commented 1 year ago

Thanks for the detailed info. Helped me understand the situation a bit more clearly.