-
Notifications
You must be signed in to change notification settings - Fork 0
Updated server logic #36
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
base: st_server_logic
Are you sure you want to change the base?
Conversation
…ge the users database, small bugfixes on the DatabaseManager
| */ | ||
|
|
||
| /*Questions I have | ||
| - why do we do ByteString.copyFrom instead of just converting to bytestring directly |
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.
If possible, switch to doing <proto>.toByteString(), may cut out a needless copy (unsure if it is doing that)
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.
Complete
| - why do we do ByteString.copyFrom instead of just converting to bytestring directly | ||
| - ChannelActive is called twice when I run my testing client | ||
| - The things the client sends are only handled by one instance, so it is not *Currently* a problem | ||
| - Why does this happen? |
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.
Requires investigation from where netty is creating the ServerHandler (Server.java)
| private String selfName = ""; | ||
| private MessageManager mgr = new MessageManager(); | ||
| private MessageManager msgmgr = new MessageManager(); | ||
| private DatabaseHandler dbmgr = new DatabaseHandler("Users.db"); //Can change where the database is stored later |
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.
if not already present, add *.db to gitignore to be safe
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.
Is a case by case thing? Or can you configure git to ignore all.db files? I did not push the database itself.
|
|
||
| import java.util.ArrayList; | ||
|
|
||
| public class SessionData { |
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.
This class needs threadsafety
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.
Added a synchronized tag to addClientUser. Resolved, maybe
| .setType(Control.ServerMessageType.SESSION_REJECTED) | ||
| .setSuccess(false) | ||
| .build(); | ||
| sendProto(ByteString.copyFrom(reply.toByteArray()), General.FvPacketType.SERVER_MESSAGE); |
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.
This is sending the "rejected" message back to the prospective host, not the person who requested.
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.
Resolved
| .setType(Control.ServerMessageType.SESSION_STARTING) | ||
| .setSuccess(true) | ||
| .build(); | ||
| sendProto(ByteString.copyFrom(reply.toByteArray()), General.FvPacketType.SERVER_MESSAGE, UsersData.getUser(sr.getName())); |
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.
In the case of a "race condition" here, do some error checking to ensure that the requester still exists.
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.
same as L212, 213
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.
What is the race condition here? I'm not sure I understand. I added a check at the beginning of the method to ensure that the "client" still exists by the time the server receives the host's response packet.
Resolved (maybe).
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.
that check should resolve it, race condition in quotes meant it was an instance where an unexpected change occurred under your nose
| .setType(Control.ServerMessageType.SESSION_STARTING) | ||
| .setSuccess(true) | ||
| .build(); | ||
| sendProto(ByteString.copyFrom(reply.toByteArray()), General.FvPacketType.SERVER_MESSAGE, UsersData.getUser(sr.getName())); |
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.
See comment from L209
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.
See above. Resolved (maybe)
No description provided.