Skip to content
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

Redefinition of ThreadingHTTPServer in server.py and server-r.py #35

Open
BuongiornoTexas opened this issue Apr 12, 2023 · 4 comments
Open

Comments

@BuongiornoTexas
Copy link

Hi @jasonacox,

I'm not quite sure what is happening here, but the code in the server modules is doing something odd. Using server.py as the example:

  • Line 19 imports ThreadingHTTPServer
    from http.server import BaseHTTPRequestHandler, HTTPServer, ThreadingHTTPServer
    
  • Lines 110-112 appear to redefine ThreadingHTTPServer
    class ThreadingHTTPServer(ThreadingMixIn, HTTPServer):
        daemon_threads = True
        pass
    
  • Line 347 instantiates the result.

From a quick object inspection, line 347 creates server with base classes of HTTPServer and ThreadingMixIn (from line 110) - which is not helpful, because these are the base classes we would expect from the redefinition and also the base classes for ThreadingHTTPServer ...

Is this what you intended or should lines 110-112 be removed and line be modified to:

from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer

I'm guessing the latter?

(This is an aside triggered by the ToU stuff I'm putting together - which unfortunately could be made thread unsafe very easily!).

@jasonacox
Copy link
Owner

Lines 110-112 appear to redefine ThreadingHTTPServer

That's a class override to inject daemon_threads = True but I'll need to investigate when I get some time. Are you seeing any odd behavior?

Thanks @BuongiornoTexas .

@BuongiornoTexas
Copy link
Author

I'm not seeing any odd behaviour.

But isn't line 110 re-declaring rather than over-riding ThreadingHTTPServer? That is, this creates a new class type inheriting from HTTPServer and ThreadingMixIn (i.e. discarding the imported definition and replacing it with the new class). That's certainly the warning I'm getting from mypy.

Your declaration should give the same result as the import - I checked the library module, and the definition of the imported ThreadingHTTPServer is the same as your redeclaration.

class ThreadingHTTPServer(socketserver.ThreadingMixIn, HTTPServer):
    daemon_threads = True

So not an issue now, just an oddity.

@BuongiornoTexas
Copy link
Author

I just did what I should of have done in the first place and tested commenting out lines 110-112. From a quick and dirty check, it looks as though the server operates as expected - in particular threads fire up and don't block.

Given I've gone a PR for ToU that includes minor changes to server.py, are you happy for me to roll this into that PR?

@jasonacox
Copy link
Owner

Yes, please do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants