py-mine / mcstatus

A Python library for checking the status of Minecraft servers
https://mcstatus.readthedocs.io
Apache License 2.0
468 stars 37 forks source link

JavaServer ping() always fails? ("did not respond with any information") #850

Open katrinafyi opened 1 month ago

katrinafyi commented 1 month ago

ping doesn't seem to work for any servers I've tested, most of which return:

OSError: Server did not respond with any information!
```python Traceback (most recent call last): File "/home/rina/progs/mcstatus/mcstatus/__main__.py", line 104, in main() File "/home/rina/progs/mcstatus/mcstatus/__main__.py", line 100, in main args.func(server) File "/home/rina/progs/mcstatus/mcstatus/__main__.py", line 11, in ping print(f"{server.ping()}ms") ^^^^^^^^^^^^^ File "/home/rina/progs/mcstatus/mcstatus/server.py", line 94, in ping return self._retry_ping(connection, **kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/rina/progs/mcstatus/mcstatus/utils.py", line 67, in sync_wrapper raise last_exc # type: ignore # (This won't actually be unbound) ^^^^^^^^^^^^^^ File "/home/rina/progs/mcstatus/mcstatus/utils.py", line 63, in sync_wrapper return func(*args, **kwargs) ^^^^^^^^^^^^^^^^^^^^^ File "/home/rina/progs/mcstatus/mcstatus/server.py", line 100, in _retry_ping return pinger.test_ping() ^^^^^^^^^^^^^^^^^^ File "/home/rina/progs/mcstatus/mcstatus/pinger.py", line 63, in test_ping response = self.connection.read_buffer() ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/rina/progs/mcstatus/mcstatus/protocol/connection.py", line 319, in read_buffer length = self.read_varint() ^^^^^^^^^^^^^^^^^^ File "/home/rina/progs/mcstatus/mcstatus/protocol/connection.py", line 257, in read_varint part = self.read(1)[0] ^^^^^^^^^^^^ File "/home/rina/progs/mcstatus/mcstatus/protocol/connection.py", line 549, in read raise IOError("Server did not respond with any information!") OSError: Server did not respond with any information! ```

For example:

poetry run python -m mcstatus mc.hypixel.net ping
poetry run python -m mcstatus mccentral.org ping
poetry run python -m mcstatus demo.mcstatus.io ping
poetry run python -m mcstatus play.purpleprison.net ping

Note, this was tested through the CLI but I can't see why it would be CLI specific.

katrinafyi commented 1 month ago

After poking in Wireshark, I can't find a discrepancy. The Python is sending the handshake and ping packet correctly. The server is closing the connection without sending any bytes, and the Python is handling that closure.

PerchunPak commented 1 month ago

can reproduce

katrinafyi commented 1 month ago

Is a status packet required before ping? Wiki.vg says no, but performing a status first in the connection seems to fix it. I don't know if this caused by the protocol or a socket bug in mcstatus.

PerchunPak commented 1 month ago

Apparently, we already saw this bug in https://github.com/py-mine/mcstatus/pull/491. worth looking for why it wasn't already fixed on the server implementations side

Not closing this, because we should probably add a note to docs about this.

katrinafyi commented 1 month ago

I see, would it be a good idea to implement ping in using status, then? Maybe ping can automatically fallback to status.

I imagine this is what the vast majority of users would prefer. For ping purposes, you could abort the connection after reading the status response ID to avoid deserialising the whole response.

kevinkjt2000 commented 1 month ago

Closing as a duplicate of #491 Use latency from status object or your own timings as a workaround. We are not able to help with all the non-vanilla implementations behave differently.

katrinafyi commented 1 month ago

I don't see a point in clinging onto a function that works on only a fraction of active servers? Regardless of technical correctness, this is a disaster for the user experience, and at the very least needs an easily discoverable warning.

ItsDrike commented 1 month ago

I do agree that we can add a note/warning to the ping docstring, as it can be weird to users that it doesn't work when the server is up and status does work. However, this really is an issue with the upstream server implementations. It is them who don't follow the spec properly.

Making an additional status request before ping is very wasteful from our side and using status packet instead of ping for the ping method makes no sense, at that point, just use status. At most, we could add a bool arg that would make this extra status request in ping, ignore it and then perform ping. But I don't love that idea.

We already manually count the time it took for the server to respond when you're using status, so you can just use the latency parameter from that if you need it.

We could also consider deprecating ping entirely and just keeping status, considering it contains the latency anyways.

katrinafyi commented 1 month ago

I'd support implementing ping() entirely in terms of a status packet. I don't see a need to remove ping(), introducing a breaking change and pushing this task onto every user of the library.

With an implementation as status().latency, the basic ping would depend on the well-formedness of the status response which might not be desirable. This is why I suggested ping() sends a status packet but ignores the data payload of the response, thus acting in a different way to status().latency.

Finally, there is a rare possibility, mentioned in https://github.com/py-mine/mcstatus/pull/491#issuecomment-1440484840, where a server might support ping but not status. It would be useful to keep the real ping accessible for such cases (either through a flag or different method name).

Personally, I think performing an automatic fallback (try ping, then status) would be best. This would allow you to print an informative warning if the first ping fails, so users know to file an issue with the server software. I don't feel strongly about fallback vs only using status, however.

PerchunPak commented 1 month ago

Reopening this, since discussion is not over and we definitely have to add a note/warning to docs about this.

Anyway, I agree with ItsDrike, it's server implementations who don't follow the spec, and appropriate bug report should be opened at their repos. ping() is designed to be as simple as possible, and there is no need to over design it with status().

katrinafyi commented 1 month ago

You could even re-raise the OSError with more informative/friendly error text which would put the information somewhere users will definitely see.

PerchunPak commented 1 month ago

Yeah, that is even better solution. If I won't forget, I will create a PR for this

kevinkjt2000 commented 1 month ago

I also agree it would be wasteful to request full status from the server and not use it, hence ping should be separate and standalone. Adding the documentation tag for this, but I have concern that this particular OSError is indistinguishable from legitimate cases where the server crashes or a firewall drops the response.

katrinafyi commented 1 month ago

Sure, you could report to the user "if ping fails but status succeeds, it is likely .....".

PerchunPak commented 1 month ago

You can check against error message