PaperMC
Feature request: Expose a way to know if server is shutting down through API
Makes sense to have reliable way to know if velocity is shutting down in the API.
My personal use case
I have logic that works with database on player disconnect event. When velocity shuts down I'd like to make one final database request and close the server but that's require my own AtomicBoolean handler to figure out if server shuts down at the player disconnect event which is performance costly (Since I need a really reliable way to do this)
Alternatively before I just left everything to player disconnect event (which is called on shutdown to everyone) but it makes shutdown process longer than 10-30 seconds depending on the load
UPD: Actually Velocity uses it's own AtomicBoolean. I fail see reason why plugin developers should spawn additional Atomics to figure out if server shutting down while being in other events/any other part of code besides ProxyShutdownEvent
ProxyShutdownEvent?
I literally explained that what I'm using this right now. I have my own AtomicBoolean that gets to true on ProxyShutdownEven. But why should I handle this myself when there is private AtomicBoolean in VelocityServer class? It would be much easier to just have isShutdown() method and it makes sense, to me at least
I can't see a valid justification here other than "if you add this I'll be able to optimize my code." To me, this sounds like a completely broken design on your part, that the process of committing changes after a player disconnect is extremely expensive.
It's not that I'm opposed to adding this API, but the reason you've provided here is extremely weak.
I believe plugin developers can be in situations where they need to know the current status of a proxy (e.g. shuts down or not).
ProxyShutdownEvent does the trick, developer can catch if proxy shuts down on it's own and act upon it in their code, but let's say I have over 10 plugins and each one of them tracks shutdowns on it's own. To me this sounds like a "possible" issue. I doubt any Velocity setup really run amount of plugins that can lead to significant performance differences. But I consider this the kind of change that is "nice to have".
To me, this sounds like a completely broken design on your part
Isn't async nature of events in Velocity is intended for a developer to actually run performance hungry things right in the event?
More to that I have just two plugins, one of them tracks and synchronizes player-list across multiple instances using Redis, which I do not consider heavy. Other tracks time in remote SQL database. I can implement a separate async pipeline that would be collecting needed data and will be sending data in batches. And make ProxyShutdownEvent force all the pending data to be pushed at once. But this setup sounds to me unreasonably complex for Velocity plugin and looks like premature optimization. First of all such setup would make sense under high loads. In my environment about a year ago before I moved to multi-instances my server was barely under 1.5k (it was laggy - main reason we turned to horizontal scaling back then). We've setup 4 instances of Velocity each of them was servicing barely 400 players at once. Does it really worth setting up additional layer of abstraction just for 400 players? I think not. Both jedis and HikariCP (and I assume any other good library for any database) support pooling and thus allow running queries from multi-threaded environment that Velocity is. There is no need for complex abstraction, because under high load it can get laggy anyway.
Also this API doesn't even require much changes and it costs almost nothing to add. Personally I can't see a valid justification for not adding it in the first place besides my argument being weak (which is true, as of writing this I can't make up any serious example that would make change necessary). But I believe this change can be beneficial to someone in the future
Isn't async nature of events in Velocity is intended for a developer to actually run performance hungry things right in the event?
This is correct, but there are still certain constraints that Velocity cannot eliminate for you. If the process of saving player data to the DB is too expensive, the onus is on you to optimize that by doing things like using indices and performing operations in batch if need be.
Also this API doesn't even require much changes and it costs almost nothing to add.
But it would certainly cost us in the long term when it comes to maintaining it. No API is truly free, which is why I want to see a justification for each addition. So far I haven't seen such a case.
If the process of saving player data to the DB is too expensive
Sorry for not being clear enough process of saving data to the DB on it's own is not expensive and not the issue. (I again go back to my personal case but bear with me for a second) Issue is long shutdown process because it runs disconnect event for each player that was on the instance at once (fetches connection form the pool, runs single statement and etc.) as if he disconnected normally and we can't differentiate through API if disconnection was because server shuts down or player did it on it's own. Right now it can be solved by plugin's own shutdown flag which is weird while we have an API that can in fact shutdown the proxy (but can't tell if it's in process of shutting down)
UPD: Also just tested ProxyShutdownEvent. Disconnect is called earlier then shutdown so there is actually now way to catch a moment through API where proxy in process of shutting down and players are yet to disconnect. So plugin's own AtomicBoolean is not even an option anymore.
The ProxyShutdownEvent is intended for plugins to clean up their resources, so firing disconnect events afterwards is not an option. There are ways to detect the shutdown:
a. Call ProxyServer#shutdown within your plugin after performing the DB batch update. The volatile read in the disconnect event is negligible.
b. Override the shutdown command, and perform the same logic as in (a).
I must agree these aren't ideal (especially b).
