Handle legacy server list pings

[rj00a]

Describe the problem related to your feature request.

pre-Netty server list pings are not currently handled.

https://github.com/valence-rs/valence/blob/cd7dd8cc4cd6fe7765f82f0e2c6bdcf3dac887a1/crates/valence/src/server/connect.rs#L102-L104

Modern clients can still send the ping in some circumstances so we should deal with it.

This might be the source of an issue I've seen where the client will be timed out waiting for a response.

Describe the solution you would like.

Respond to the status ping in the expected way. The AsyncCallbacks trait might need adjustment to account for this.

Additional context

https://wiki.vg/Server_List_Ping#1.6

[PonasKovas]

@rj00a I could probably take on this, since I recently implemented this on my own project.

Just to make sure: you want a different callback for legacy pings in NetworkCallbacks, or somehow reuse the normal ping callback?

Note that legacy ping responses have pretty strict limits: the length of the whole string payload (MotD, online players, max players, protocol and version combined) must be under ~256 characters (exact number varies). If longer, the client just shows "Communication error" on the server list.

So I personally would favor the separate callback choice and that's how I did it in my project, but since I'm not very familiar with this project yet, I'd thought I'd ask for your opinion before starting to work on this :slightly_smiling_face:

[PonasKovas]

Also, do you think it should validate (for length) the legacy ping response before sending them? What should happen if the response is too long to be valid? An error with tracing and just don't send it?

[dyc3]

There's an invalid length for legacy pings? I think the best way to handle that is to truncate whatever text is causing it to be too long.

[PonasKovas]

I guess you could truncate the longest field to make it valid in most cases, but what if multiple fields are too long?

Note that the length which matters for validity is not of individual fields (like motd, etc) but the sum of all fields combined.

[rj00a]

So I personally would favor the separate callback choice

Agree

Note that the length which matters for validity is not of individual fields (like motd, etc) but the sum of all fields combined.

In that case a tracing warning or some smart constructor on the return type of the callback seem like good choices. As long as it doesn't silently send invalid data and the constraint is documented somewhere.

[PonasKovas]

Since the version and protocol are constants, and online_players and max_players are integers (maximum 11 chars in string form), it seems to me that the best solution is to just truncate the description if needed and write a warning that it was done.

Also another issue is that none of the fields can contain the § character (and it can't be escaped lol), so I guess it should just check and remove them from the text before sending the package (and a warning of course)?

[rj00a]

it seems to me that the best solution is to just truncate the description if needed and write a warning that it was done.

Alright

Also another issue is that none of the fields can contain the § character

Isn't § used for formatting codes in the MOTD? So I don't think you would want to remove them.

[PonasKovas]

Isn't § used for formatting codes in the MOTD? So I don't think you would want to remove them.

Ah yeah, I kind of forgot about that haha. Pre-1.4 legacy ping uses § as a separator for the fields and doesn't support formatting, so that confused me a bit.

Now I actually changed my opinion about the callback. I think that we should just reuse the same ping callback and just try to respond with something as close as possible to what the actual response would be. So that would mean truncating the description if too long, removing § if it's pre-1.4, and attempting to convert the Chat JSON object to a string with §s instead for 1.4-1.6 (not sure how trivial that is, have not looked into how Chat is implemented here yet).

I don't think anyone cares about legacy pings, they just need to work. As far as I've looked even spigot has no separate event for the legacy ping.

What are your thoughts?

[rj00a]

Since the version and protocol are constants

I think the legacy server ping callback should have the ability to change the reported version string, which would make it not a constant anymore. The regular server ping callback should be able to do that too, but the functionality is just missing currently.

Now I actually changed my opinion about the callback. I think that we should just reuse the same ping callback and just try to respond with something as close as possible to what the actual response would be

One solution here is to provide a legacy callback method in NetworkCallbacks but give it a default implementation which tries to reuse the regular callback. This would give users the ability to override it if desired.

removing § if it's pre-1.4

I had assumed we would only support the 1.6 legacy ping, since that's only what the current vanilla server supports? But if vanilla does support pre-1.6 pings, then perhaps we could have separate methods for each of those.

So that would mean truncating the description if too long, removing § if it's pre-1.4, and attempting to convert the Chat JSON object to a string with §s instead for 1.4-1.6 (not sure how trivial that is, have not looked into how Chat is implemented here yet).

Also keep in mind that the string needs to be converted to UTF-16BE with null characters removed, since that is used as the separator. Regular UTF-8 strings in Rust are allowed to contain null bytes.

[dyc3]

I had assumed we would only support the 1.6 legacy ping

I agree with this. I think this is a very reasonable scope constraint. Valence is aiming for the latest version of vanilla MC. Trying to support all the possible versions of legacy pings is a waste of time IMO.

[PonasKovas]

One solution here is to provide a legacy callback method in NetworkCallbacks but give it a default implementation which tries to reuse the regular callback. This would give users the ability to override it if desired.

Sounds good.

I had assumed we would only support the 1.6 legacy ping, since that's only what the current vanilla server supports? But if vanilla does support pre-1.6 pings, then perhaps we could have separate methods for each of those.

I actually just did some testing with the straight vanilla 1.20.1 server fresh from the official website. Turns out it DOES NOT in fact respond to the 1.6 legacy ping, even though the client attempts it if the normal ping fails just like described in wiki.vg. However, it does support the earlier (pre-1.4, 1.4-1.5) legacy pings. Classic Mojang...

Most other servers that are not straight vanilla fresh from the official website, do respond to all of the legacy pings correctly.

That said, I think that there should be 2 callbacks, one for normal ping, one for legacy, which would be modeled around the 1.6 format, and by default reuse the normal ping callback. If pre-1.4 legacy ping occurs, it would just remove formatting.

[PonasKovas]

I had assumed we would only support the 1.6 legacy ping

I agree with this. I think this is a very reasonable scope constraint. Valence is aiming for the latest version of vanilla MC. Trying to support all the possible versions of legacy pings is a waste of time IMO.

I wouldn't say it's an overkill. It's not a lot of extra work to support all of them.

Plus it's much easier to check if a Minecraft server is online using the pre-1.4 format, you just send a single byte and parse the response. You never know if some old minecraft server list website isn't still using that lol :grin:

[rj00a]

Most other servers that are not straight vanilla fresh from the official website, do respond to all of the legacy pings correctly.

That said, I think that there should be 2 callbacks, one for normal ping, one for legacy, which would be modeled around the 1.6 format, and by default reuse the normal ping callback. If pre-1.4 legacy ping occurs, it would just remove formatting.

Alright, sounds good. Thanks for investigating 👍🏻.