-
Notifications
You must be signed in to change notification settings - Fork 1
Fix isSystemPacket, refactor net code. #136
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
Conversation
|
|
||
| public abstract class AbstractPacket { | ||
| public interface Type { | ||
| int getType(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused imports
Is Type ever going to be needed outside AbstractPacket
Can we name the nested interface "PacketType" instead, to disambiguate from other Types
Maybe getType instead of getId?
| public static PacketType fromInteger(int type) throws InvalidPacketException { | ||
| switch (type) { | ||
| case 0: return PING; | ||
| case 1: return CLOSE_CONNECTION; | ||
| default: throw new InvalidPacketException("Invalid PacketType"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to gracefully handle this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any suggestions? Currently the hope is if someone disconnects, the other end will know either because the CloseConnectionResolver gets called and closes the connection, or the heartbeat doesn't get a response and it closes that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So sorry I put comment on wrong line, should be line 50
throw new InvalidPacketException("Invalid PacketType");
Having this in mekwars/common might be ok... Do all modules know they have to handle the InvalidPacketException?
I guess my concern was that the InvalidPacketException is thrown and crashes the client or server because it propagates to the top of the stack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, these methods only get called by Connection#readObject, which only gets called by Connection#read, which fizzles any packet that doesn't have a valid packet type.
sandysimonton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At your discretion
a22019a to
57f18f9
Compare
57f18f9 to
56143ba
Compare
This PR does the following:
isSystemPacketcheck never worked as thePingResolvernever explicitly checked if the packet was a system packet, causing the first registered packed to be interpreted as a ping.Connectionconstructors that would partially setup the connection then finish setting up withonConnect. Instead create theConnectionadd listeners, then connect withconnect. This also removes theonConnectmethod and puts it into theconnectmethod.CloseConnectionA packet that declares the other side is disconnecting.CallbackResolvercode into theAbstractResolverso you can do callbacks on every resolver now.