Unable to get status from recent version of Velocity server

[clrxbl]

Not sure why but connect.2b2t.org specifically does not seem to respond to mcstatus and I'm not getting blocked by their proxy or TCPShield. It seems to be able to get a response from websites like https://mcsrvstat.us, but fails with mcstatus.

Traceback (most recent call last):
  File "/opt/homebrew/lib/python3.11/site-packages/mcstatus/server.py", line 128, in status
    return self._retry_status(connection, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/lib/python3.11/site-packages/mcstatus/utils.py", line 66, in sync_wrapper
    raise last_exc  # type: ignore # (This won't actually be unbound)
    ^^^^^^^^^^^^^^
  File "/opt/homebrew/lib/python3.11/site-packages/mcstatus/utils.py", line 62, in sync_wrapper
    return func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/lib/python3.11/site-packages/mcstatus/server.py", line 134, in _retry_status
    result = pinger.read_status()
             ^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/lib/python3.11/site-packages/mcstatus/pinger.py", line 114, in read_status
    response = self.connection.read_buffer()
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/lib/python3.11/site-packages/mcstatus/protocol/connection.py", line 313, in read_buffer
    length = self.read_varint()
             ^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/lib/python3.11/site-packages/mcstatus/protocol/connection.py", line 254, in read_varint
    part = self.read(1)[0]
           ^^^^^^^^^^^^
  File "/opt/homebrew/lib/python3.11/site-packages/mcstatus/protocol/connection.py", line 539, in read
    raise IOError("Server did not respond with any information!")
OSError: Server did not respond with any information!

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/opt/homebrew/bin/mcstatus", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/opt/homebrew/lib/python3.11/site-packages/mcstatus/__main__.py", line 101, in main
    args.func(server)
  File "/opt/homebrew/lib/python3.11/site-packages/mcstatus/__main__.py", line 15, in status
    response = server.status()
               ^^^^^^^^^^^^^^^
  File "/opt/homebrew/lib/python3.11/site-packages/mcstatus/server.py", line 127, in status
    with TCPSocketConnection(self.address, self.timeout) as connection:
  File "/opt/homebrew/lib/python3.11/site-packages/mcstatus/protocol/connection.py", line 519, in __exit__
    self.close()
  File "/opt/homebrew/lib/python3.11/site-packages/mcstatus/protocol/connection.py", line 512, in close
    self.socket.shutdown(socket.SHUT_RDWR)
OSError: [Errno 57] Socket is not connected

[PerchunPak]

Discussed on Discord, actual traceback is:

Traceback (most recent call last):
  File "/app/src/logic/get_status.py", line 54, in get_status
    status = await server.async_status(tries=1)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/mcstatus/server.py", line 147, in async_status
    return await self._retry_async_status(connection, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/mcstatus/utils.py", line 52, in async_wrapper
    raise last_exc  # type: ignore # (This won't actually be unbound)
    ^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/mcstatus/utils.py", line 48, in async_wrapper
    return await func(*args, **kwargs)  # type: ignore # (We know func is awaitable here)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/mcstatus/server.py", line 153, in _retry_async_status
    result = await pinger.read_status()
             ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/mcstatus/pinger.py", line 107, in read_status
    return JavaStatusResponse.build(raw, latency=(received - start) * 1000)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/mcstatus/status_response.py", line 134, in build
    motd=Motd.parse(raw["description"], bedrock=False),
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/mcstatus/motd/__init__.py", line 52, in parse
    parsed = cls._parse_as_dict(raw, bedrock=bedrock)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/mcstatus/motd/__init__.py", line 137, in _parse_as_dict
    parsed_motd.extend(cls._parse_as_dict(element, auto_add=auto_add.copy()))
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/mcstatus/motd/__init__.py", line 113, in _parse_as_dict
    if (color := item.get("color")) is not None:
                 ^^^^^^^^
AttributeError: 'str' object has no attribute 'get'

To summarize, we get OSError because we retry too fast. For the first two attempts, we got an exception on parsing, and on the third retry, the server bans us because of spamming. The same issue with the retry decorator happened already in #511.

We can fix it (retry decorator) by removing it or by retrying only the actual logic (not parsing). To be honest, this kind of logic should be on the user, not on us.

[kevinkjt2000]

I recall the number of attempts is adjustable. It should be possible to set that to 1 and skip our retry logic.

[ItsDrike]

Yeah, we could just set the retry amount to 1 by default, but I think we should also make sure the retry logic only runs on the actual connection, and not on parsing logic.

However I'm also not opposed to just removing the retry logic entirely.

[PerchunPak]

We can also set the default value to None (which would mean one) for the deprecation period and raise a warning if the retry value is not None. Then, after the deprecation period, remove the retry logic entirely.

[ItsDrike]

I would prefer setting it to a custom private sentinel object, say like this:

from typing import NewType

_SENTINEL_T = NewType("_SENTINEL_T", object)
_SENTINEL = _SENTINEL_T(object())

def some_func(tries: int | _SENTINEL_T = _SENTINEL):
    if tries is _SENTINEL:
        tries = 1  # or the original 3
    else:
        deprecation_warn()
    ...

This way no one will leave a some_func(tries=None) in their code, as that would cause issues once removed. It's just a small nitpicky edge-case, but I'd rather see this approach, as it would be more reliable. But yeah in general, the principle is the same.

There is still the alternative of just setting the default to 1 and making sure we're retrying only the connection logic, instead of completely dropping retries. But yeah, I'm not sure they're worth keeping around.

[PerchunPak]

I also thought about sentinel, but then I realized that None is just for such cases (as the default value). Anyway, will use sentinel, so it is harder for a user to shoot their own leg.