Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
katrinafyi opened this issue Jul 16, 2024 · 15 comments
Open
Labels
area: documentation Related to project's documentation (comments, docstrings, docs)

Comments

@katrinafyi
Copy link
Contributor

katrinafyi commented Jul 16, 2024

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
Copy link
Contributor Author

katrinafyi commented Jul 16, 2024

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 PerchunPak added the type: bug Something isn't working label Jul 16, 2024
@PerchunPak
Copy link
Member

can reproduce

@katrinafyi
Copy link
Contributor Author

katrinafyi commented Jul 16, 2024

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
Copy link
Member

Apparently, we already saw this bug in #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
Copy link
Contributor Author

katrinafyi commented Jul 16, 2024

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
Copy link
Contributor

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 kevinkjt2000 added meta: duplicate This issue or pull request already exists and removed type: bug Something isn't working labels Jul 17, 2024
@katrinafyi
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

katrinafyi commented Jul 17, 2024

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 #491 (comment), 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
Copy link
Member

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 PerchunPak reopened this Jul 17, 2024
@katrinafyi
Copy link
Contributor Author

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

@PerchunPak
Copy link
Member

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

@kevinkjt2000 kevinkjt2000 added area: documentation Related to project's documentation (comments, docstrings, docs) and removed meta: duplicate This issue or pull request already exists labels Jul 17, 2024
@kevinkjt2000
Copy link
Contributor

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
Copy link
Contributor Author

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

@PerchunPak
Copy link
Member

You can check against error message

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: documentation Related to project's documentation (comments, docstrings, docs)
Projects
None yet
Development

No branches or pull requests

4 participants