Skip to content

Conversation

@Biquaternions
Copy link

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 PaperServerListPingEventImpl and StandardPaperServerListPingEventImpl to 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#processRequest can 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:

  1. It handles adventure components, which are not precisely lightweight.
  2. Existing plugins haven't been made considering the possibility of this event being synchronous.

Using MCUtil could be a solution, but I couldn't find scenarios where it was used for a similar purpose.

@Biquaternions Biquaternions requested a review from a team as a code owner January 26, 2026 00:00
@github-project-automation github-project-automation bot moved this to Awaiting review in Paper PR Queue Jan 26, 2026
@mbax
Copy link
Contributor

mbax commented Jan 26, 2026

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.

@Biquaternions
Copy link
Author

Biquaternions commented Jan 26, 2026

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)
I think the markdown caused the url to not be easily visible.

In that branch, the event was split into PaperServerListPingEvent and ServerListJoinEvent.
If you think that approach is still valid despite the duplicated code it causes in StandardPaperServerListPingEventImpl#getListedPlayers, StandardPaperServerListPingEventImpl#getPlayerSample and StandardPaperServerListPingEventImpl#getPlayerSampleHandle I can still bring it back to this PR.

@Biquaternions
Copy link
Author

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:

  1. Making a separate event that will trigger only when the player joins.
  2. Using the same event that will be called synchronously.

From these two, this PR contributes [2], which expands the event to be triggered sync/async and adds the documentation you mentioned.
The reason why I ended up discarding [1], which you can still find in this branch (c3d023f) is basically repeated logic:

The current architecture for PaperServerListPingEvent looks like this:
architecture_0

I want to clarify that the implementation I made on the old branch is incomplete, the static method in ServerListJoinEventImpl can be isolated in a different class and shared with StandardPaperServerListPingEventImpl.
But a tentative final version would have the following architecture:
architecture_1

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.
If this does not represent a problem, then I can update this PR to bring back [1].

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 ServerListPingEvent or PaperServerListPingEvent will work immediately, only requiring an update to handle the aforementioned sync/async scenario if the plugin requires it.

Still I have a couple of questions on how should I proceed with this PR, regardless if ends up being [1] or [2]:

  1. Is using MCUtil#scheduleAsyncTask appropriate in this case? The fix in Fire ServerListPingEvent for secondary motd send (fixes #8073) #8074 uses it to make the call async, which I find appropriate since adventure components are a bit heavy, and that event in particular can be a little heavy in general, since some people use PaperServerListPingEvent to hide players, which in cases where synchronization is required, will involve database operations, which should not be on the main thread. However, there's not many instances of MCUtil#scheduleAsyncTask being used for the purpose of "skipping a beforehand known to be heavy task".
  2. If implementation [1] were to stay, where should I move the static method processRequest that both ServerListJoinEventImpl and StandardPaperServerListPingEventImpl will share? What package is most suited for that, and under what class name?

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.

@electronicboy
Copy link
Member

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

@Biquaternions
Copy link
Author

Firing a previously async event as sync when we're expecting people to do IO operations in there is a no go

I agree on that, only reason I took that approach was due to being used in a similar event (AsyncChatEvent).
So this means I should probably bring back and make a variation of implementation [1] I mentioned in the last reply.

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

As for this, I personally don't think there could be much abuse considering this event would be fired after all the configuration and connection validation, it will only be triggered right before PlayerJoinEvent.
Still, taking your approach I understand I should create a dedicated (and probably bounded to a single thread) pool to execute this logic, which I would probably create statically inside ServerListJoinEventImpl (or the utility class that will share the method ´processRequest´ for both StandardPaperServerListPingEventImpl and ServerListJoinEventImpl) since I don't think a new pool would fit inside MCUtil.

This motd data is a bit of a doozy because it has some different data expectations and an entirely different lifecycle

This is something I also noticed, even though it is a server status packet, it fires during the player join logic, it would even be a Player event rather than a Server one, depending on how you look at it.
What do you think of this last part?

Now, to conclude and before I start rewriting the PR, what architecture suits the use case for this new event better?
I have this two proposals, tho any modification is welcomed. The methods displayed are the ones that will get repeated depending on the approach.

Architecture A

architecture_0

Architecture B

architecture_1

The ServerListJoinEvent could even be replaced by a PlayerListPingEvent considering it is fired for a specific player when they join the server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Awaiting review

Development

Successfully merging this pull request may close these issues.

RGB colors in MOTD not supported when player is kicked

3 participants