mcstatus icon indicating copy to clipboard operation
mcstatus copied to clipboard

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

Open katrinafyi opened this issue 1 year ago • 15 comments
trafficstars

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

OSError: Server did not respond with any information!
Traceback (most recent call last):
  File "/home/rina/progs/mcstatus/mcstatus/__main__.py", line 104, in <module>
    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 avatar Jul 16 '24 12:07 katrinafyi

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.

katrinafyi avatar Jul 16 '24 13:07 katrinafyi

can reproduce

PerchunPak avatar Jul 16 '24 14:07 PerchunPak

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.

katrinafyi avatar Jul 16 '24 14:07 katrinafyi

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.

PerchunPak avatar Jul 16 '24 15:07 PerchunPak

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.

katrinafyi avatar Jul 16 '24 15:07 katrinafyi

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.

kevinkjt2000 avatar Jul 17 '24 06:07 kevinkjt2000

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.

katrinafyi avatar Jul 17 '24 06:07 katrinafyi

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.

ItsDrike avatar Jul 17 '24 10:07 ItsDrike

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.

katrinafyi avatar Jul 17 '24 10:07 katrinafyi

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().

PerchunPak avatar Jul 17 '24 11:07 PerchunPak

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

katrinafyi avatar Jul 17 '24 11:07 katrinafyi

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

PerchunPak avatar Jul 17 '24 12:07 PerchunPak

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.

kevinkjt2000 avatar Jul 17 '24 13:07 kevinkjt2000

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

katrinafyi avatar Jul 17 '24 13:07 katrinafyi

You can check against error message

PerchunPak avatar Jul 17 '24 14:07 PerchunPak