Always ping server before initiating connection?

[deathcap]

ping(options, cb) performs a Server List Ping using the STATUS protocol, returning a bunch of useful information, example:

{
    "version": {
        "name": "1.8.7",
        "protocol": 47
    },
    "players": {
        "max": 100,
        "online": 5,
        "sample": [
            {
                "name": "thinkofdeath",
                "id": "4566e69f-c907-48ee-8d71-d7ba5aa00d20"
            }
        ]
    },  
    "description": {
        "text": "Hello world"
    },
    "favicon": "data:image/png;base64,<data>"
}

The version/protocol field could be useful for #234 Cross-version protocol support. Before initiating the connection, perform a ping to get the version and use it accordingly. description, favicon, and other fields could also be saved for consultation later.

FML/Forge servers #114 #326 additionally contain more information:

  "modinfo": {
    "type": "FML",
    "modList": [
      {
        "modid": "mcp",
        "version": "9.18"
      },
      {
        "modid": "FML",
        "version": "8.0.99.99"
      },
      {
        "modid": "Forge",
        "version": "11.15.0.1715"
      },
      {
        "modid": "IronChest",
        "version": "6.0.121.768"
      }
    ]
  }

(modinfo.type is also returned on Glowstone++, 'BUKKIT' is a recognized type, to show the Bukkit logo in FML's server list screen. 'VANILLA' as well but not clear if anyone sends it.).

modList is particularly useful because it gives the list of mods you must have in order to connect to the server. The FML|HS handshake protocol sends this list in the ModList packet, and if it doesn't match the server will kick the client, so the client would have to know it a priori unless retrieved from the Server List Ping.

createClient() already does perform one pre-connect action: resolving DNS srv (_minecraft._tcp) records. ping.js doesn't do this, but it probably should (bug? anyone want to test?). Server List Ping would be another reasonable pre-connect step to perform, imho. Real clients usually show a server list before the player connects, so this would more closely match vanilla behavior as well.

Therefore here's the changes I suggest:

• Refactor DNS SRV resolution out of createClient.js, so ping() can use it standalone as well
• Enhance createClient() to ping() (including DNS SRV) before connecting, by default
• Optionally allow createClient() to skip pinging (option ping: false?), if desired; probably ought to also have dns_srv: false option to skip the pre-connect DNS resolution step. Some clients may want to skip these steps (and forgo the added benefits - no server description, automatic protocol negotiation, automatic Forge mod support), but I'd imagine most will want the useful default functionality

[deathcap]

Made this change in examples/client_forge: 283b75d - but I think it'd be useful generically

[roblabla]

ping not doing DNS SRV is a bug.

The ability to disable the SRV request isn't super useful in the 99.9999% scenario. The notchian client makes an SRV DNS request, we're just matching that. If someone wants to bypass it, he can just provide an already-created TCP stream to NMP (when we support that).

IMO, createClient shouldn't call ping. The notchian client doesn't do that, for one. For two, some servers don't reply to pings. What you describe is a hack specific for the forge client, and I think should only belong to a future "nmp-forge" module. The overhead isn't negligeable (initiate a connection, handshake, ping, close connection) for something that people not connecting to forge (which should be most people) don't need at all.

[rom1504]

"ping not doing DNS SRV is a bug." Actually it does, connect is defined in
createClient and ping is only used in files including createClient (in
test.js). That means ping doesn't work if you require only ping.js but that
shouldn't happen since users require the whole nmp. That design is a bit
confusing though, could be improved.

On Sun, Jan 24, 2016, 14:03 Robin Lambertz [email protected] wrote:

ping not doing DNS SRV is a bug.

The ability to disable the SRV request isn't super useful in the 99.9999%
scenario. The notchian client makes an SRV DNS request, we're just matching
that. If someone wants to bypass it, he can just provide an already-created
TCP stream to NMP (when we support that).

IMO, createClient shouldn't call ping. The notchian client doesn't do
that, for one. For two, some servers don't reply to pings. What you
describe is a hack specific for the forge client, and I think should only
belong to a future "nmp-forge" module. The overhead isn't negligeable
(initiate a connection, handshake, ping, close connection) for something
that people not connecting to forge (which should be most people) don't
need at all.

—
Reply to this email directly or view it on GitHub
#327 (comment)
.

[rom1504]

Pinging before connecting could be an option in createClient, because it's
indeed useful to support dynamic cross version. We could have a timeout for
it so that if it timeout something appropriate could be done (defaulting to
the current version for dynamic cross version or trying normal login for
forge (or a default list of mods)

On Sun, Jan 24, 2016, 14:28 Romain Beaumont [email protected] wrote:

"ping not doing DNS SRV is a bug." Actually it does, connect is defined in
createClient and ping is only used in files including createClient (in
test.js). That means ping doesn't work if you require only ping.js but that
shouldn't happen since users require the whole nmp. That design is a bit
confusing though, could be improved.

On Sun, Jan 24, 2016, 14:03 Robin Lambertz [email protected]
wrote:

ping not doing DNS SRV is a bug.

The ability to disable the SRV request isn't super useful in the 99.9999%
scenario. The notchian client makes an SRV DNS request, we're just matching
that. If someone wants to bypass it, he can just provide an already-created
TCP stream to NMP (when we support that).

IMO, createClient shouldn't call ping. The notchian client doesn't do
that, for one. For two, some servers don't reply to pings. What you
describe is a hack specific for the forge client, and I think should only
belong to a future "nmp-forge" module. The overhead isn't negligeable
(initiate a connection, handshake, ping, close connection) for something
that people not connecting to forge (which should be most people) don't
need at all.

—
Reply to this email directly or view it on GitHub
#327 (comment)
.

[deathcap]

If a server does not respond to Server List Ping, then how will it appear in the server list? Example of server that doesn't respond?

update: edited src/createServer.js, commented out client.once('ping_start', onPing); to test against the vanilla client - it will show "Pinging…" and a red X with a "no connection" tooltip:

and then after some time "Can't connect to server":

although you can still double-click to force connect

[deathcap]

Relevant: https://github.com/Dinnerbone/mcstatus

[roblabla]

I've seen some custom servers do that to put a server in some sort of
"maintenance mode"

On Sun, Jan 24, 2016, 5:15 PM deathcap [email protected] wrote:

Relevant: https://github.com/Dinnerbone/mcstatus

—
Reply to this email directly or view it on GitHub
#327 (comment)
.

[deathcap]

Here's a working prototype along the lines of what I had in mind:

https://github.com/deathcap/node-minecraft-protocol-auto

Uses a ping to get the protocol version, enables Forge with mod list if server type='FML', and also automatically sets the client protocol version to match the server version. Works in testing a 1.9 snapshot, 1.8.9, some third-party 1.8 server software, and a Forge 1.8.9 server, but has some rough edges:

• Hardcodes knowledge of release versions and protocol versions, would be nice to get this from minecraft-data instead (protocolVersions.json added in https://github.com/deathcap/node-minecraft-protocol-forge)
• Uses only the major release version (1.8, 1.9..), but client/server may have mismatched protocol versions, PR to NMP to allow specifying protocolVersion directly: [WIP] Intra-release multi-protocol support #330
• Ping timeouts: not handled well, could fallback to a normal server connection if an error is received, but if the server is up but ignoring the ping then it'll wait indefinitely, separate bug Missing ping timeout callback when server is running but not responding to pings #329
• Had to change the API of the wrapper, not compatible with client = createClient() because of the ping packet, made it async createClientAsync(options, function(err, client)
• Forge support uses node-minecraft-protocol-forge, TBD how this support will fit in Forge client support (support integrating with node-minecraft-protocol-forge) #326

I haven't published node-minecraft-protocol-auto to NPM, I still think it would be a worthwhile addition to node-minecraft-protocol itself. Maybe not in createClient(), but it could be a separate API available as an opt-in feature.

[deathcap]

This issue is well-covered by #234 so I'll close it, still would like to see this though =)