-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Fix custom MOTD not being sent after a player joins the server (fixes #8073) #13582
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
base: main
Are you sure you want to change the base?
Conversation
|
Maybe I'm blind but I don't see the new event you describe making. Instead I see a javadoc addition about sync fires of the event being for different scenarios. |
As mentioned, I ended up not taking that approach, but the implementation is still available in this branch (c3d023f) In that branch, the event was split into PaperServerListPingEvent and ServerListJoinEvent. |
|
I took the time to explain this a little bit further. I gave it a quick read and noticed I didn't properly explained the reasoning behind the PR. To summarize, I made two attempts:
From these two, this PR contributes [2], which expands the event to be triggered sync/async and adds the documentation you mentioned. The current architecture for I want to clarify that the implementation I made on the old branch is incomplete, the static method in As shown, there are 3 methods which will be repeated and, while they're not long, they add a maintainability cost since if one were to have a bug, the team would have to remember the other event exists and manually update it to match the first one. Implementation [2] is easier to maintain since is the same event, but requires manually handling by the developer if the event was fired sync/async. It also fixes immediately the issue for all existing plugins since it doesn't require a separate event, which means all plugins using either Still I have a couple of questions on how should I proceed with this PR, regardless if ends up being [1] or [2]:
This PR is very much open for discussion, I'd greatly appreciate if I can receive some feedback on what approach would be the best between [1] and [2], and if I could get some directions regarding the questions I have. |
|
Firing a previously async event as sync when we're expecting people to do IO operations in there is a no go MCUtil#scheduleAsyncTask is our general purpose thread pool for stuff which doesn't need or warrant its own seperate thread pool, organisation, constraining, etc; This sorta thing often wants to be on some form of bounded pool to prevent abuse This motd data is a bit of a doozy because it has some different data expectations and an entirely different lifecycle, using the existing event always felt heavily flawed for this reason; at the very least we need to expose that this is being fired from a non-status reason |




Fixes #8073 and is based on #8074.
As recommended in #8074 I tried creating a new event to handle the two cases with different events, as they have different behaviors.
However, given the logic in
PaperServerListPingEventImplandStandardPaperServerListPingEventImplto handle the original event, it is impossible to follow a similar structure without having duplicated code somewhere.This is the attempt I made, where some functions contain duplicated code, which will make it harder to maintain. (The method
StandardPaperServerListPingEventImpl#processRequestcan still be isolated tho)c3d023f
The current implementation in this PR has the drawback of firing the event on the main thread when the player joins (other scenarios are still async), which I'm not a big fan of, specially since:
Using MCUtil could be a solution, but I couldn't find scenarios where it was used for a similar purpose.