-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Some possible SampHubServer improvements #2321
Description
I've been looking over the code for SampHubServer more carefully than I have in the past, partially motivated by issues like #2064, and have come up with some ideas to help improve and perhaps simplify the code, making it easier to debug.
I don't have time to go about implementing them right now but I wanted to write down my current thoughts before I forget:
1 . The repeated calls to select seem to be problematic to me. For example, before allowing any requests to be handled by the web client server there's first a select call on just the standard client server with an 0.01 second timeout. That means even if there are only web clients they always have to have that extra delay while waiting on the standard server to poll. The reverse is also true--if there are only standard clients, but the web profile server is running, then they always have to wait on the web profile server's select.
There main server loop should just have one call to select with the listen sockets for both servers in readfds, and then call the corresponding handle_request for each server with an incoming request.
2 . Furthermore, the default handle_request method for TCPServer also calls select for that server's listen socket, with its own timeout handling. Since we only call handle_request after our own poll this should not time out and should return fairly quickly. That said, it's superfluous and we could override our ThreadedXMLRPCServer.handle_request to just call _handle_request_noblock() immediately.
3 . Sort of a side issue, but I don't like how web_profile_text_dialog handles user input directly in the server loop, causing it to block. Perhaps it would be better if it registered some kind of callback, and added sys.stdin to the file handles to select. Each callback registered to the server would be associated with a given file handle. So web_profile_text_dialog would just add sys.stdin and register a callback on it to do something once input is received from the user. This callback mechanism should probably be publicly exposed somehow. (One issue to note is that select on sys.stdin won't work on Windows. Perhaps for Windows, and only Windows, we could have a popup dialog as the default or something, or just let it block...)
ETA: Ah, I knew there was another issue I was forgetting:
4 . Is there really a need for dedicated threads for checking hub and client timeout? Couldn't those checks also be handled just in the main server loop? That would seem good enough to me...