Add dynamic cross-protocol support #331

deathcap · 2016-01-25T02:57:16Z

Implements the dynamic part of #234 Cross version support

Ported from https://github.com/deathcap/node-minecraft-protocol-auto (which I can deprecate if this is merged)

To do:

Enhance createClient() to allow waiting/pausing so it can be reconfigured before connect -- src/client/setProtocol.js? (installs client.on('connect') listener)
Change the protocol negotiation to be supported with a createClient()-compatible API (vs createClientAsync)
Update to use minecraft-data protocolVersions
Move into src/client/ to act as plugin/middleware

rom1504 · 2016-01-25T08:58:03Z

createClient doesn't create an "all ready" client: people will want to wait for the login packet for example.
So I think it would be okay to return a client synchronously, and do the ping and set protocol thingies inside of createClient "afterwards".
That would mean changes in Client, but it might be okay.
This would have the benefit of keeping just one createClient.

rom1504 · 2016-01-25T09:01:11Z

Err probably not login, that's server side, but there are many "ready" events in mineflayer (spawn, received inventory,...), so people wait for one of these before writing anything to the client.
It's also important to be able to listen to client "error" event asap

deathcap · 2016-01-26T02:33:45Z

I'd definitely like to maintain API compatibility with createClient, will have to investigate more to try to do this. The problem I was running into is that the client created by createClient would continue the protocol handshake even before I received the ping. Maybe need some way to "pause" the client object? Also to dynamically change the version and hostname after object construction.

rom1504 · 2016-01-26T10:15:16Z

I think to do it cleanly you'd have to change createClient so it would
(optionally) "wait" (listen to the right event,...) for the ping pong to be
over before starting the handshake. It's createClient doing the state
changing, Client only provide the functionality to do so.
Also I think it would be possible that way to use the same socket for
pinging and for the rest. The vanilla client probably doesn't use the same
socket, but I think it makes sense.
Yeah it would be needed to change the version after the pong. It is kind of
already being done anyway since mc.ping default to 1.8.

On Tue, Jan 26, 2016, 03:33 deathcap [email protected] wrote:

I'd definitely like to maintain API compatibility with createClient, will
have to investigate more to try to do this. The problem I was running into
is that the client created by createClient would continue the protocol
handshake even before I received the ping. Maybe need some way to "pause"
the client object? Also to dynamically change the version and hostname
after object construction.

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

deathcap · 2016-01-28T07:45:34Z

Yeah it would be needed to change the version after the pong. It is kind of
already being done anyway since mc.ping default to 1.8.

I think that's problematic for other reasons, imho the 0xfe01 ping #332 is superior, as it does not require a status handshake, nor a protocol version (and works on all 1.4.4+)

I think to do it cleanly you'd have to change createClient so it would
(optionally) "wait" (listen to the right event,...) for the ping pong to be
over before starting the handshake. It's createClient doing the state
changing, Client only provide the functionality to do so.

Notes for testing this scenario:

edit src/createServer.js to comment out client.once('ping_start', onPing);

node examples/server/server.js

NODE_DEBUG=mc-proto node examples/client_auto/client_auto.js localhost 25565

client should hang (hits #329 Missing ping timeout callback when server is running but not responding to pings) or timeout:

node-minecraft-protocol $ NODE_DEBUG=mc-proto node examples/client_auto/client_auto.js localhost 25565
MC-PROTO: 54954 creating client
MC-PROTO: 54954 pinging localhost
MC-PROTO: 54954 writing packet handshaking.set_protocol
MC-PROTO: 54954 { protocolVersion: 47,
  serverHost: 'localhost',
  serverPort: 25565,
  nextState: 1 }
MC-PROTO: 54954 writing packet status.ping_start
MC-PROTO: 54954 {}

then edit src/createClientAuto.js (from this PR), move the createClient() call outside of the ping callback directly into createClientAsync(), and rerun client_auto.js

now, the client will despite not receiving a ping (callback not executed), it will still begin to connect:

node-minecraft-protocol $ NODE_DEBUG=mc-proto node examples/client_auto/client_auto.js localhost 25565
MC-PROTO: 54970 creating client
MC-PROTO: 54970 pinging localhost
MC-PROTO: 54970 writing packet handshaking.set_protocol
MC-PROTO: 54970 { protocolVersion: 47,
  serverHost: 'localhost',
  serverPort: 25565,
  nextState: 2 }
MC-PROTO: 54970 writing packet login.login_start
MC-PROTO: 54970 { username: 'echo' }
MC-PROTO: 54970 writing packet handshaking.set_protocol
MC-PROTO: 54970 { protocolVersion: 47,
  serverHost: 'localhost',
  serverPort: 25565,
  nextState: 1 }
MC-PROTO: 54970 writing packet status.ping_start
MC-PROTO: 54970 {}
MC-PROTO: 54970 read packet login.compress
MC-PROTO: 54970 { threshold: 256 }
MC-PROTO: 54970 read packet login.success
MC-PROTO: 54970 { uuid: 'c39fa4c6-85a3-3582-8167-db3805dbe851',
  username: 'echo' }
MC-PROTO: 54970 read packet play.chat
MC-PROTO: 54970 { message: '{"translate":"chat.type.text","with":["Server","echo joined the game."]}',
  position: 0 }
MC-PROTO: 54970 read packet play.login
MC-PROTO: 54970 { entityId: 1,
  gameMode: 1,
  dimension: 0,
  difficulty: 2,
  maxPlayers: 127,
  levelType: 'default',
  reducedDebugInfo: false }
MC-PROTO: 54970 read packet play.position
MC-PROTO: 54970 { x: 0, y: 256, z: 0, yaw: 0, pitch: 0, flags: 0 }
MC-PROTO: 54970 read packet play.keep_alive
MC-PROTO: 54970 { keepAliveId: 2138214823 }
MC-PROTO: 54970 writing packet play.keep_alive
MC-PROTO: 54970 { keepAliveId: 2138214823 }
MC-PROTO: 54970 read packet play.keep_alive
MC-PROTO: 54970 { keepAliveId: 648251752 }
MC-PROTO: 54970 writing packet play.keep_alive
MC-PROTO: 54970 { keepAliveId: 648251752 }
MC-PROTO: 54970 read packet play.keep_alive
MC-PROTO: 54970 { keepAliveId: 1917937053 }
MC-PROTO: 54970 writing packet play.keep_alive
MC-PROTO: 54970 { keepAliveId: 1917937053 }

Now that the client is more modular, it's easier to see what's going on, need to test to confirm but I think this is where it is unexpectedly (or undesirably) executing from:

// src/client/setProtocol.js
ar states = require("../states");

module.exports = function(client, options) {
  client.on('connect', onConnect);

  function onConnect() {
    client.write('set_protocol', {
      protocolVersion: options.protocolVersion,
      serverHost: options.host,
      serverPort: options.port,
      nextState: 2
    });

setProtocol sends the set_protocol packet as soon as the socket connects.

(and src/client/caseCorrect.js calls client options.connect(), which is set from src/client/tcp_dns.js to call client.setSocket())

deathcap · 2016-01-28T08:02:06Z

caseCorrecting and tcp_dns (SRV lookup and TCP initiation) can occur beforehand, but sending set_protocol should be delayed until the client knows (via ping) the protocolVersion.

deathcap · 2016-01-28T08:58:59Z

Listen for another event in setProtocol before sending the packet? 'connect' is going to be received when the TCP socket it connected, whether we are ready for it or not. Additionally block on another event (optionally), something like 'connect_allowed'?

…c/client/setProtocol proceeds Intended to allow clients to configure the client object before any data is sent, specifically, before src/client/setProtocol sends the set_protocol packet.

deathcap · 2016-01-31T05:45:23Z

Also I think it would be possible that way to use the same socket for
pinging and for the rest. The vanilla client probably doesn't use the same
socket, but I think it makes sense.

The server immediately closes the socket after sending the ping response, so unfortunately I don't think reusing it is possible.

This allows client plugins to change the options on the client object, and have it reflected in other plugins. Previously, had to rely on the options being the same object, and hold a reference to it. Now it is always accessible as `client.options`.

deathcap · 2016-01-31T06:52:48Z

Converted to use "sync" API ok, started to move into createClient() via a src/client/autoVersion.js plugin. Having some difficulty re-versioning the client object after it is instantiated. The constructor calls setHandshake() and sets .version to the major version, so I do this too in autoVersion.js after the version is ready, and then emit connect_allowed to uncork setProtocol, it sends the set_protocol packet but then the program just exits:

node-minecraft-protocol $ NODE_DEBUG=mc-proto node examples/client_auto/client_auto.js localhost 9999
MC-PROTO: 7249 creating client
MC-PROTO: 7249 pinging localhost
connected
MC-PROTO: 7249 writing packet handshaking.set_protocol
MC-PROTO: 7249 { protocolVersion: 47,
  serverHost: 'localhost',
  serverPort: 9999,
  nextState: 1 }
MC-PROTO: 7249 writing packet status.ping_start
MC-PROTO: 7249 {}
MC-PROTO: 7249 read packet status.server_info
MC-PROTO: 7249 { response: '{"description":{"text":"A Minecraft Server"},"players":{"max":20,"online":0},"version":{"name":"15w40b","protocol":76}}' }
MC-PROTO: 7249 writing packet status.ping
MC-PROTO: 7249 { time: [ 0, 0 ] }
MC-PROTO: 7249 read packet status.ping
MC-PROTO: 7249 { time: [ 0, 0 ] }
MC-PROTO: 7249 ping response { description: { text: 'A Minecraft Server' },
  players: { max: 20, online: 0 },
  version: { name: '15w40b', protocol: 76 },
  latency: 1 }
MC-PROTO: 7249 Server description: { text: 'A Minecraft Server' }
MC-PROTO: 7249 Server version: 15w40b, protocol: 76
{ minecraftVersion: '15w40b',
  version: 76,
  usesNetty: true,
  majorVersion: '1.9' }
MC-PROTO: 7249 writing packet handshaking.set_protocol
MC-PROTO: 7249 { protocolVersion: 76,
  serverHost: 'localhost',
  serverPort: 9999,
  nextState: 2 }
MC-PROTO: 7249 writing packet login.login_start
MC-PROTO: 7249 { username: 'echo' }

deathcap · 2016-01-31T19:39:17Z

Looks like the ping connection is somehow interfering with the subsequent normal connection. connected is logged right after "pinging localhost", when receiving the client connect event, but not for the re-connect with the new version. The program ends because the client receives the end event.

The logged connected message is actually from the normal connection, the TCP socket connects almost immediately, but is held without reply until the ping is received. client_echo logs for comparison:

node-minecraft-protocol $ NODE_DEBUG=mc-proto node examples/client_echo/client_echo.js localhost 18900
setProtocol onConnect
MC-PROTO: 11592 writing packet handshaking.set_protocol
MC-PROTO: 11592 { protocolVersion: 47,
  serverHost: 'localhost',
  serverPort: 18900,
  nextState: 2 }
MC-PROTO: 11592 writing packet login.login_start
MC-PROTO: 11592 { username: 'echo' }
connected
MC-PROTO: 11592 read packet login.compress
MC-PROTO: 11592 { threshold: 256 }
MC-PROTO: 11592 read packet login.success
MC-PROTO: 11592 { uuid: 'c39fa4c6-85a3-3582-8167-db3805dbe851',
  username: 'echo' }
MC-PROTO: 11592 read packet play.login
MC-PROTO: 11592 { entityId: 364,
  gameMode: 0,
  dimension: 0,
  difficulty: 1,
  maxPlayers: 20,
  levelType: 'default',
  reducedDebugInfo: false }
MC-PROTO: 11592 read packet play.custom_payload
MC-PROTO: 11592 { channel: 'MC|Brand', data: <Buffer 07 76 61 6e 69 6c 6c 61> }
MC-PROTO: 11592 read packet play.difficulty
MC-PROTO: 11592 { difficulty: 1 }
MC-PROTO: 11592 read packet play.spawn_position
MC-PROTO: 11592 { location: { x: -8, y: 64, z: -196 } }

Problem with the serializer? Packet capture shows the handshaking.set_protocol packet is never sent on the 2nd connection, only the login.login_start packet:

00000000  06                                               .
00000001  00 04 65 63 68 6f                                ..echo

even though it was logged as being written in:

setting serializer for state=handshaking, version=1.9
MC-PROTO: 12311 writing packet handshaking.set_protocol
MC-PROTO: 12311 { protocolVersion: 76,
  serverHost: 'localhost',
  serverPort: 9999,
  nextState: 2 }
setting serializer for state=login, version=1.9
MC-PROTO: 12311 writing packet login.login_start
MC-PROTO: 12311 { username: 'echo' }
ended

…re properly piped

deathcap · 2016-01-31T20:22:24Z

Updating the client state using client.state = states.HANDSHAKING instead of client.setSerializer(states.HANDSHAKING) resolves the above issue, now able to connect with the auto-negotiated protocol.

(why I was confused: the src/client.js constructor calls this.setSerializer, not this.state=..., tried to mimic that behavior to re-initialize the version, but set state(newProperty) has a setter to perform important useful tasks of piping/unpiping the serializers).

…inecraft-data

deathcap · 2016-02-01T04:53:02Z

Failing tests in requiring minecraft-data: PrismarineJS/node-minecraft-data#16

Attempted to use latest node-minecraft-data from git, but still fails, I think CircleCI is caching the node_modules. Will try a clean CircleCI setup.

PrismarineJS/node-minecraft-protocol#331 Add dynamic cross-protocol support

…-data#16

rom1504 · 2016-02-01T08:44:12Z

src/client/caseCorrect.js

@@ -1,7 +1,8 @@
 var yggdrasil = require('yggdrasil')({});
 var UUID = require('uuid-1345');

-module.exports = function(client, options) {
+module.exports = function(client) {
+  var options = client.options;


Why client.options ? The client, options pattern is consistent with what mineflayer is doing, and it seems to makes sense. (It also allows destructuring : for example https://github.com/PrismarineJS/flying-squid/blob/master/src/lib/plugins/world.js#L10 )

Ah I saw your commit message "This allows client plugins to change the options on the client object,
and have it reflected in other plugins. Previously, had to rely on the
options being the same object, and hold a reference to it. Now it is
always accessible as client.options."

It sounds like it assume some order of execution between plugins. Which plugins is accessing the modified property ?

ah that kind of thing https://github.com/PrismarineJS/node-minecraft-protocol/pull/331/files#diff-9053b72ce1984293dcb954a0ccc89097R9.
Couldn't it be put in a client.wait_connect ?
But the problem of relying on the order of execution of plugins remains.

rom1504 · 2016-02-01T08:58:57Z

I made some comments

minecraft-data 0.19.1 is now usable
I'd like to keep the client, options pattern if possible, I think it would be better to put the changing attributes in the Client object https://github.com/PrismarineJS/node-minecraft-protocol/blob/master/src/client.js#L14

(I guess it's probably necessary for some plugins to depend on other, but let's probably keep it implicit for now)

roblabla · 2016-02-01T11:54:42Z

I am very, very, very much against the client.options pattern. Ideally, I should be able to do

var client = new Client();
tcpConnect(client, { opts for tcpConnect });
forgeHandshake(client, { opts for forgeHandshake });
play(client, { opts for play });
etc...

What plugin needs modifying the options ? I believe that if any plugin requires this, it should instead wrap the original plugin in some way.

deathcap · 2016-02-01T17:52:01Z

Updated to data 0.19.1, removing need for git clone post-action

Plugins currently modifying options:

autoVersion: sets options.wait_connect, used by setProtocol plugin
caseCorrect: sets accessToken and haveCredentials
tcp_dns: sets port and host to defaults if not set, and sets connect to callback
forgeHandshake: sets tagHost

I suppose these 'options' could be moved onto the client object, since they're not really options but data passed between plugins (which can initially come from options, but only as a default). tagHost could use the system described in #336 (comment)

rom1504 · 2016-02-01T18:32:04Z

src/client/autoVersion.js

+
+// Get the minecraft-data version string for a protocol version
+// TODO: add to node-minecraft-data index (protocol to newest release, if multiple)
+function protocolVersion2MinecraftVersion(n) {


this can now be implemented using https://github.com/PrismarineJS/node-minecraft-data/blob/master/example.js#L19 I think

ah, missed that, will use postNettyVersionsByProtocolVersion

deathcap · 2016-02-02T05:18:57Z

Alright I've restored the options parameters to each of the client plugin parameters. This gets us closer to separate independent options per plugin, but a few plugins still modify their options making them interdependent.

deathcap · 2016-02-02T05:44:26Z

Moved wait_connect from options to the client object, but moving all the other plugin-mutable options to client is a much bigger change, would prefer it as a separate PR (poses some non-obvious questions, e.g. how to handle the version parameter in the options vs the client object)

rom1504 · 2016-02-02T08:04:18Z

Ok good enough. We can track the remaining things to do here in #338 and #234

Add dynamic cross-protocol support

This was referenced Jan 26, 2016

[WIP] Intra-release multi-protocol support #330

Closed

versionsByProtocolVersion index (complement versionsByMinecraftVersion index for protocolVersions lookup) PrismarineJS/node-minecraft-data#15

Closed

This was referenced Jan 27, 2016

Modularization attempt of createClient() #333

Merged

Forge client support (support integrating with node-minecraft-protocol-forge) #326

Merged

Add dynamic cross-protocol support

a53a297

deathcap force-pushed the dynxver branch from a2b0d7c to a53a297 Compare January 28, 2016 07:29

Add wait_connect option, to require 'connect_allowed' event before sr…

9ec6d87

…c/client/setProtocol proceeds Intended to allow clients to configure the client object before any data is sent, specifically, before src/client/setProtocol sends the set_protocol packet.

deathcap added 2 commits January 30, 2016 22:09

Fix unintentional var client declaration

00bf6ac

Update to use compatible sync API (createClient)

66e6f4d

deathcap force-pushed the dynxver branch from ef86f63 to 66e6f4d Compare January 31, 2016 06:16

deathcap added 4 commits January 30, 2016 22:27

Use minecraft-data for protocol version -> minecraft version

89198c6

Move options to client object

ea3b306

This allows client plugins to change the options on the client object, and have it reflected in other plugins. Previously, had to rely on the options being the same object, and hold a reference to it. Now it is always accessible as `client.options`.

Rename src/createClientAuto.js -> src/client/autoVersion.js

af62189

Remove useless statement

163e215

Support options.version==false in createClient() to auto-detect version

9a68fd6

deathcap force-pushed the dynxver branch from d25d76a to 9a68fd6 Compare January 31, 2016 06:53

Set state property instead of setSerializer() to ensure serializers a…

d0f64e9

…re properly piped

deathcap added 2 commits January 31, 2016 12:26

Use new object for ping() to avoid potential cross-contamination

f17de64

Update client connect() for options move compatibility

2e2a213

Update to minecraft-data ^0.19.0

84c4c26

deathcap mentioned this pull request Feb 1, 2016

0.19.0 release: Error: Cannot find module './minecraft-data/data/common/protocolVersions' PrismarineJS/node-minecraft-data#16

Closed

Update minecraft-data to use GitHub dependency on PrismarineJS/node-m…

4b3f8b5

…inecraft-data

deathcap force-pushed the dynxver branch from 801598f to 4b3f8b5 Compare February 1, 2016 04:43

Try using git commit hash to node-minecraft-data

aa3ae32

deathcap added a commit to deathcap/node-minecraft-protocols that referenced this pull request Feb 1, 2016

Merge branch 'dynxver'

ba89e24

PrismarineJS/node-minecraft-protocol#331 Add dynamic cross-protocol support

deathcap force-pushed the dynxver branch 2 times, most recently from 20564a4 to 80fe58f Compare February 1, 2016 05:26

Try to clone minecraft-data to workaround PrismarineJS/node-minecraft…

070a046

…-data#16

deathcap force-pushed the dynxver branch from 80fe58f to 070a046 Compare February 1, 2016 05:49

rom1504 reviewed Feb 1, 2016
View reviewed changes

Update to minecraft-data ^0.19.1

c2913da

rom1504 reviewed Feb 1, 2016
View reviewed changes

deathcap added 2 commits February 1, 2016 21:02

Lookup version using minecraft-data postNettyVersionsByProtocolVersion

58fd1ea

Move options back to client parameters

a8bc7d1

Move wait_connect to client object

a3a2d3d

rom1504 mentioned this pull request Feb 2, 2016

Move all the plugin-mutable options to client #338

Open

rom1504 added a commit that referenced this pull request Feb 2, 2016

Merge pull request #331 from deathcap/dynxver

0ba187d

Add dynamic cross-protocol support

rom1504 merged commit 0ba187d into PrismarineJS:master Feb 2, 2016

rom1504 mentioned this pull request Feb 2, 2016

much let, very const #337

Merged

This was referenced Feb 7, 2016

Multi-protocol Support PrismarineJS/mineflayer#199

Closed

Enable auto-version support including for Forge voxel/voxel-clientmc#24

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dynamic cross-protocol support #331

Add dynamic cross-protocol support #331

deathcap commented Jan 25, 2016

rom1504 commented Jan 25, 2016

rom1504 commented Jan 25, 2016

deathcap commented Jan 26, 2016

rom1504 commented Jan 26, 2016

deathcap commented Jan 28, 2016

deathcap commented Jan 28, 2016

deathcap commented Jan 28, 2016

deathcap commented Jan 31, 2016

deathcap commented Jan 31, 2016

deathcap commented Jan 31, 2016

deathcap commented Jan 31, 2016

deathcap commented Feb 1, 2016

rom1504 Feb 1, 2016

rom1504 Feb 1, 2016

rom1504 Feb 1, 2016

rom1504 commented Feb 1, 2016

roblabla commented Feb 1, 2016

deathcap commented Feb 1, 2016

rom1504 Feb 1, 2016

deathcap Feb 2, 2016

deathcap commented Feb 2, 2016

deathcap commented Feb 2, 2016

rom1504 commented Feb 2, 2016

Add dynamic cross-protocol support #331

Add dynamic cross-protocol support #331

Conversation

deathcap commented Jan 25, 2016

rom1504 commented Jan 25, 2016

rom1504 commented Jan 25, 2016

deathcap commented Jan 26, 2016

rom1504 commented Jan 26, 2016

deathcap commented Jan 28, 2016

deathcap commented Jan 28, 2016

deathcap commented Jan 28, 2016

deathcap commented Jan 31, 2016

deathcap commented Jan 31, 2016

deathcap commented Jan 31, 2016

deathcap commented Jan 31, 2016

deathcap commented Feb 1, 2016

rom1504 Feb 1, 2016

Choose a reason for hiding this comment

rom1504 Feb 1, 2016

Choose a reason for hiding this comment

rom1504 Feb 1, 2016

Choose a reason for hiding this comment

rom1504 commented Feb 1, 2016

roblabla commented Feb 1, 2016

deathcap commented Feb 1, 2016

rom1504 Feb 1, 2016

Choose a reason for hiding this comment

deathcap Feb 2, 2016

Choose a reason for hiding this comment

deathcap commented Feb 2, 2016

deathcap commented Feb 2, 2016

rom1504 commented Feb 2, 2016