Debugger hang fix #7
Conversation
dashmips/debuggerserver.py
Outdated
| def handle(self): | ||
| # self.request is the TCP socket connected to the client | ||
| while True: | ||
| data = self.request.recv(65000).strip() |
There was a problem hiding this comment.
We should be using the size to recv the correct amount of bytes here. you'll want to read in the header which is delimited by { to } and use the value of size to recv the remaining message
There was a problem hiding this comment.
There might be a more clean way to use recv, but I just received 1 byte using a while loop to get the size string. This did allow me to remove the regular expressions from the python code, which makes it overall cleaner.
| while True: | ||
| header = b"" | ||
| while True: | ||
| header += self.request.recv(1) | ||
| if header and chr(header[-1]) == '}': | ||
| break |
There was a problem hiding this comment.
I'm fine with this method, let us be a little more defensive and add a condition that if header is 1000 characters and we still haven't seen the closing curly we should exit with with failure.
There was a problem hiding this comment.
How would you prefer the program exit? Should it throw an exception and log an error message?
There was a problem hiding this comment.
Good question, makes me realize we might have added a bit of a bug here with program exiting in general, the old client loop function used to break the websocket loop which implicitly would lead to an exit. My advice it to have one actual sys.exit() point in the program and to reach that point via exceptions.
So I think try: except:-ing around serve_forever is appropriate, with except blocks for KeyboardInterruption, ProgramExit and CommunicationError (or ParseError, you decide) (the last two being exceptions of your own making) and handle them appropriately (for example program exit isn't actually a bad thing, we just want consistent exit strategies)
You should log what you think is best so here's a note about levels:
Will only be seen if someone passed -l to dashmips:
debug- for a developer to get detailed insight on the internals of a program (like dump of network traffic)info- verbose information, like entering certain blocks of code or calling functions, intended for someone debugging an issue in productionwarning- not an error but something that should be corrected
Will always print:
criticalorerror- something unexpected occurred the program cannot continue, this is in the realm of socket port in use, or permission denied, things entirely unfixable by our program
There was a problem hiding this comment.
I committed the changes I made, it should now exit with a message on both normal completion and if the message is too long (> 1000). Let me know if you still want me to add an exit for KeyboardInterruption!
dashmips/debuggerserver.py
Outdated
| print("{} wrote:".format(self.client_address[0])) | ||
| print(command) |
There was a problem hiding this comment.
lets search and replace prints with log.info or log.debug, this prevents clutter on the terminal when someone runs dashmips without the -l flag
| while True: | ||
| header = b"" | ||
| while True: | ||
| header += self.request.recv(1) | ||
| if header and chr(header[-1]) == '}': | ||
| break |
There was a problem hiding this comment.
Good question, makes me realize we might have added a bit of a bug here with program exiting in general, the old client loop function used to break the websocket loop which implicitly would lead to an exit. My advice it to have one actual sys.exit() point in the program and to reach that point via exceptions.
So I think try: except:-ing around serve_forever is appropriate, with except blocks for KeyboardInterruption, ProgramExit and CommunicationError (or ParseError, you decide) (the last two being exceptions of your own making) and handle them appropriately (for example program exit isn't actually a bad thing, we just want consistent exit strategies)
You should log what you think is best so here's a note about levels:
Will only be seen if someone passed -l to dashmips:
debug- for a developer to get detailed insight on the internals of a program (like dump of network traffic)info- verbose information, like entering certain blocks of code or calling functions, intended for someone debugging an issue in productionwarning- not an error but something that should be corrected
Will always print:
criticalorerror- something unexpected occurred the program cannot continue, this is in the realm of socket port in use, or permission denied, things entirely unfixable by our program
No description provided.