py-mine / mcstatus

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

OSError: Received invalid status response packet #316

Closed kartashovio closed 1 year ago

kartashovio commented 2 years ago

I have problems with ping some servers. List of examples:

  1. 82.162.60.67:37821 (but: mcsrvstat.us)
  2. sr4.fuix.net:10002 (but: mcsrvstat.us)
  3. 88.198.55.217:25565 (but: mcsrvstat.us)
  4. moscow.gamai.ru:41001 (but: mcsrvstat.us)

File "/usr/local/lib/python3.9/site-packages/mcstatus/pinger.py", line 69, in read_status raise IOError("Received invalid status response packet.") OSError: Received invalid status response packet.

kevinkjt2000 commented 2 years ago

These servers are not responding to a test ping appropriately. Is that something that was disabled on the server's side? I'm noticing that these are non-vanilla servers.

kevinkjt2000 commented 2 years ago

Using the same stuff that JavaServer uses, it is possible to skip that ping test which populates that latency part of the status.

from mcstatus.address import Address
from mcstatus.pinger import ServerPinger
from mcstatus.protocol.connection import TCPSocketConnection

address = Address("82.162.60.67", 37821)
pinger = ServerPinger(TCPSocketConnection(address, timeout), address=address)
pinger.handshake()
status = pinger.read_status()
print(status.version.name)

Given vanilla servers and other implementations support this test ping, perhaps it's an error on the server's implementation or configuration for not responding to the test ping? Regardless, mcstatus could still handle this gracefully for skipping failed test pings during status, but others may already be depending on latency being always present in the status response. I propose the latency could take on a float("nan") value if it fails.

ItsDrike commented 2 years ago

mcstatus could still handle this gracefully for skipping failed test pings during status

I like this idea, however I think it may be a better idea to introduce a keyword argument (and it should probably be keyword-only) that skips the test_ping. We can mention this possibility by capturing the IOError from the test_ping, and reraising it with a more verbose message, including the possibility to use this kwarg.

response = server.status(skip_ping=True)

I think this makes more sense, since I'm generally of the opinion that errors should never go silent, unless explicitly requested to.

kevinkjt2000 commented 2 years ago

It's pretty common to show N/A for ping latencies on most games. Introducing a boolean flag to make a function do multiple things is worse in this case.

ItsDrike commented 1 year ago

We could make the latency optional and if an exception occurs, just set it to None, however this would be compatibility breaking, as some people may rely on it being there with status.