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

Send kick packet on End #20

Closed
roblabla opened this issue Jan 26, 2013 · 12 comments
Closed

Send kick packet on End #20

roblabla opened this issue Jan 26, 2013 · 12 comments

Comments

@roblabla
Copy link
Member

I am not 100% sure about what I'm going to say here (I'm going on what I read the code), but I believe when you end a connection, you do so by simply ending the socket with some non-minecraft-standard (e.g. : KeepAliveTimeout).
A good idea would be to modify client.end() so it actually sends a kick packet, as to gracefully end the connection.

@andrewrk
Copy link
Member

There's a slippery line to draw, which is how much work does this module do. This is one of those places where I opted for this module doing less.

I have a higher level API called mineflayer which does exactly what you said.

There are some places that this module has to understand the protocol, and that is mostly because of this module handling encryption and authentication. However, at any place I could get away with not understanding the actual game protocol, I did.

Even if you want to be nice and send the kick packet, you still want to be able to forcefully end the connection. This module lets you do both. If you want to send the kick packet, use

client.write(0xff, {reason: "client.disconnect"});

If you want to forcefully end the connection, use client.end().

This is a wontfix.

@roblabla
Copy link
Member Author

I understand that, however, your module internally kicks the client based on the keepalive in the server, which results in the inability to gracefully kick the client without modifying the library's source itself.

@andrewrk
Copy link
Member

Oh I see what you're saying. Yes I agree something is wrong with the server API - it tries to do too much and too little at the same time. Sorry to jump the gun.

@andrewrk andrewrk reopened this Jan 27, 2013
@andrewrk
Copy link
Member

Maybe this could be solved by deleting the keep alive code in the server

@andrewrk
Copy link
Member

Really any way you want to solve it, I am likely to accept a PR for it.

@mappum
Copy link
Contributor

mappum commented Jan 27, 2013

Previously, there was an option to disable the keepalive kick, but it looks like that is gone now. I believe that should be in there because it is part of the protocol, not something servers need to worry about (TCP has keepalives, but it's not like you have to worry about that when using sockets).

@roblabla
Copy link
Member Author

I suppose the question is if the automatic kicking for things should use an actual protocol packet. IMO, yes.

@mappum
Copy link
Contributor

mappum commented Jan 27, 2013

Oh, when I wrote it, it did use a kick packet, it looks like that got changed.

@roblabla
Copy link
Member Author

Also, is mineflayer also a server API ? I thought it was only for client bots.

In any way, I think it would seem logical to remap client.end to both end the stream and send the packet with the reason. This way, yes, the module does "a little more" but in the end, I think kicking on stream's end is pretty much part of the protocol (wouldn't go as far to say it officially is, but it's how it "should be").

@andrewrk
Copy link
Member

mineflayer is client only, correct. I misinterpreted this issue at first thinking you were talking about client API.

@roblabla if you submit a good PR that uses an actual protocol packet for automatic kicking like you want, I will accept it.

@andrewrk
Copy link
Member

@mappum I would also accept a PR that added an option to disable keepAlive.

@roblabla
Copy link
Member Author

Ah I see, my bad for not being clear enough. Okay, will do that once I get encryption working (almost there...)

roblabla added a commit to roblabla/node-minecraft-protocol that referenced this issue Jan 27, 2013
andrewrk pushed a commit that referenced this issue Jan 28, 2013
roblabla added a commit to roblabla/node-minecraft-protocol that referenced this issue Jan 28, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants