Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Save players before plugin disabling on server shutdown #9679

Conversation

DerEchtePilz
Copy link
Contributor

For reference: https://discord.com/channels/289587909051416579/555462289851940864/1147468559887384586

If this behaviour is intended, feel free to close this PR, but as written in that message, I think /restart and /stop should follow the same order of operations when disabling plugins and saving players.

@DerEchtePilz DerEchtePilz requested a review from a team as a code owner September 2, 2023 10:54
@DerEchtePilz
Copy link
Contributor Author

I also wanted to say that this allows the PlayerKickEvent to work with the /stop command because players are kicked before plugins get disabled and therefore the PlayerKickEvent is called.

@molor
Copy link

molor commented Sep 3, 2023

Fixes #6911
Also see the comments in it

@electronicboy
Copy link
Member

This is 100% not something that can be done mid patch release, this is more a major change in behavior in which needs to be done on a major release with proper testing to ensure that we don't break anything;

This is how this logic has worked for a decade, and while you can maybe argue that it's weird, it is a side-effect of various dozen other choices in the codebase over a decade which has left us here

@DerEchtePilz
Copy link
Contributor Author

So you say this could be added but this is more likely something for a new major MC version?
Or did you mean something else with "major release"?

@lynxplay
Copy link
Contributor

lynxplay commented Sep 3, 2023

Yea, this logic would be desirable in the long run imo, it just
a) would best fit something like, 1.21 release
b) might be best to wait for papers hardfork or be proposed to spigot.

Given that the quit/onDisable stuff has worked like this for years and most logic that relies on this logic deals with rather crucial parts of plugins (e.g. data saving) changing this mid version is rather terrible for plugin developers to update.
Additionally, given that this breaks spigot compatibility, imo it is a rough PR even during a major release, as spigot plugins would just silently fail if they rely on spigots behaviour.

@DerEchtePilz
Copy link
Contributor Author

DerEchtePilz commented Sep 3, 2023

b) might be best to wait for papers hardfork or be proposed to spigot.

#6911 suggests to add a config option. Would that, assuming this is getting merged at some point, be a feasable option until a hardfork? Or is this something that better waits until the hardfork?

@electronicboy
Copy link
Member

a config option was proposed for /stop to boot everybody, but there is no expectation that it would change event behavior, the server has stopped ticking and many things relied upon that in order to fire events, etc;

If we're going to change the behavior around this, it should just be changed without a config option, rather than having such a dangerous setting sitting around with massive side-effects

@Leguan16
Copy link
Contributor

Leguan16 commented Sep 3, 2023

I guess that would be an option. But probably only if it's disabled per default, logically.

On the other side if you enable it and have spigot plugins that rely on the old system it breaks again. So maybe an unsupported option until Hardfork?

@DerEchtePilz DerEchtePilz force-pushed the server-shutdown-operation-order branch from ea7e369 to a098559 Compare September 21, 2023 19:53
@DerEchtePilz DerEchtePilz force-pushed the server-shutdown-operation-order branch 2 times, most recently from 4e2b781 to 07d1bff Compare December 4, 2023 09:12
@DerEchtePilz DerEchtePilz force-pushed the server-shutdown-operation-order branch from 07d1bff to 1ac2eb1 Compare May 14, 2024 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

6 participants