Skip to content

Some possible SampHubServer improvements #2321

Open

Description

@embray

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...

Activity

  1. added this to the v1.0.0 milestone on May 9, 2014
  2. modified the milestones: Future, v1.0.0 on Jan 13, 2015
  3. astrofrog commented on Jun 15, 2017

    @astrofrog
    Member

    For anyone reading this issue, this is still relevant (despite the removal of SSL in #6201). But I'm marking this as low priority since it works as-is, these are just ideas for improvement.

  4. removed this from the Future milestone on Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions