-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
gh-127209: Clarify and clean up the separation between BaseServer and TCPServer #127976
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: main
Are you sure you want to change the base?
Changes from all commits
ded8bb4
0f7dd2c
6c07b1e
9634c76
910f4a7
325ec83
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,10 +20,59 @@ There are four basic concrete server classes: | |
| This uses the internet TCP protocol, which provides for | ||
| continuous streams of data between the client and server. | ||
| If *bind_and_activate* is true, the constructor automatically attempts to | ||
| invoke :meth:`~BaseServer.server_bind` and | ||
| invoke :meth:`~TCPServer.server_bind` and | ||
| :meth:`~BaseServer.server_activate`. The other parameters are passed to | ||
| the :class:`BaseServer` base class. | ||
|
|
||
| In addition to the methods and attributes inherited from :class:`BaseServer`, | ||
| :class:`TCPServer` provides: | ||
|
|
||
|
|
||
| .. method:: fileno() | ||
|
|
||
| Return an integer file descriptor for the socket on which the server is | ||
| listening. This function is most commonly passed to :mod:`selectors`, to | ||
| allow monitoring multiple servers in the same process. | ||
|
|
||
|
|
||
| .. method:: server_bind() | ||
|
|
||
| Called by the server's constructor to bind the socket to the desired address. | ||
| May be overridden. | ||
|
|
||
|
|
||
| .. attribute:: address_family | ||
|
|
||
| The family of protocols to which the server's socket belongs. | ||
| Common examples are :const:`socket.AF_INET` and :const:`socket.AF_UNIX`. | ||
|
|
||
|
|
||
| .. attribute:: socket | ||
|
|
||
| The :class:`socket.socket` object on which the server will listen for incoming requests. | ||
|
|
||
|
|
||
| .. attribute:: allow_reuse_address | ||
|
|
||
| Whether the server will allow the reuse of an address. This defaults to | ||
| :const:`False`, and can be set in subclasses to change the policy. | ||
|
|
||
|
|
||
| .. attribute:: request_queue_size | ||
|
|
||
| The size of the request queue. If it takes a long time to process a single | ||
| request, any requests that arrive while the server is busy are placed into a | ||
| queue, up to :attr:`request_queue_size` requests. Once the queue is full, | ||
| further requests from clients will get a "Connection denied" error. The default | ||
| value is usually 5, but this can be overridden by subclasses. | ||
|
|
||
|
|
||
| .. attribute:: socket_type | ||
|
|
||
| The type of socket used by the server; :const:`socket.SOCK_STREAM` and | ||
| :const:`socket.SOCK_DGRAM` are two common values. | ||
|
Comment on lines
+72
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The only values, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know. This isn't new, I just moved it. I do see that socket documentation says that out of all the possible socket types, only those two "appear to be generally useful", so I think the answer is "probably, but hard to say for sure". Maybe language like " |
||
|
|
||
|
|
||
|
|
||
| .. class:: UDPServer(server_address, RequestHandlerClass, bind_and_activate=True) | ||
|
|
||
|
|
@@ -211,13 +260,6 @@ Server Objects | |
| :attr:`server_address` and :attr:`RequestHandlerClass` attributes. | ||
|
|
||
|
|
||
| .. method:: fileno() | ||
|
|
||
| Return an integer file descriptor for the socket on which the server is | ||
| listening. This function is most commonly passed to :mod:`selectors`, to | ||
| allow monitoring multiple servers in the same process. | ||
|
|
||
|
|
||
| .. method:: handle_request() | ||
|
|
||
| Process a single request. This function calls the following methods in | ||
|
|
@@ -264,12 +306,6 @@ Server Objects | |
| Clean up the server. May be overridden. | ||
|
|
||
|
|
||
| .. attribute:: address_family | ||
|
|
||
| The family of protocols to which the server's socket belongs. | ||
| Common examples are :const:`socket.AF_INET` and :const:`socket.AF_UNIX`. | ||
|
|
||
|
|
||
| .. attribute:: RequestHandlerClass | ||
|
|
||
| The user-provided request handler class; an instance of this class is created | ||
|
|
@@ -285,36 +321,10 @@ Server Objects | |
| the address, and an integer port number: ``('127.0.0.1', 80)``, for example. | ||
|
|
||
|
|
||
| .. attribute:: socket | ||
|
|
||
| The socket object on which the server will listen for incoming requests. | ||
|
|
||
|
|
||
| The server classes support the following class variables: | ||
|
|
||
| .. XXX should class variables be covered before instance variables, or vice versa? | ||
|
|
||
| .. attribute:: allow_reuse_address | ||
|
|
||
| Whether the server will allow the reuse of an address. This defaults to | ||
| :const:`False`, and can be set in subclasses to change the policy. | ||
|
|
||
|
|
||
| .. attribute:: request_queue_size | ||
|
|
||
| The size of the request queue. If it takes a long time to process a single | ||
| request, any requests that arrive while the server is busy are placed into a | ||
| queue, up to :attr:`request_queue_size` requests. Once the queue is full, | ||
| further requests from clients will get a "Connection denied" error. The default | ||
| value is usually 5, but this can be overridden by subclasses. | ||
|
|
||
|
|
||
| .. attribute:: socket_type | ||
|
|
||
| The type of socket used by the server; :const:`socket.SOCK_STREAM` and | ||
| :const:`socket.SOCK_DGRAM` are two common values. | ||
|
|
||
|
|
||
| .. attribute:: timeout | ||
|
|
||
| Timeout duration, measured in seconds, or :const:`None` if no timeout is | ||
|
|
@@ -341,6 +351,10 @@ Server Objects | |
| socket object to be used to communicate with the client, and the client's | ||
| address. | ||
|
|
||
| An implementation of :meth:`get_request` is provided by :class:`TCPServer`. | ||
| Other classes which inherit from :class:`BaseServer` directly must provide | ||
| their own implementation. | ||
|
|
||
|
|
||
| .. method:: handle_error(request, client_address) | ||
|
|
||
|
|
@@ -382,12 +396,6 @@ Server Objects | |
| on the server's socket. May be overridden. | ||
|
|
||
|
|
||
| .. method:: server_bind() | ||
|
|
||
| Called by the server's constructor to bind the socket to the desired address. | ||
| May be overridden. | ||
|
|
||
|
|
||
| .. method:: verify_request(request, client_address) | ||
|
|
||
| Must return a Boolean value; if the value is :const:`True`, the request will | ||
|
|
@@ -697,4 +705,3 @@ The output of the example should look something like this: | |
| The :class:`ForkingMixIn` class is used in the same way, except that the server | ||
| will spawn a new process for each request. | ||
| Available only on POSIX platforms that support :func:`~os.fork`. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -161,14 +161,15 @@ class BaseServer: | |
| - __init__(server_address, RequestHandlerClass) | ||
| - serve_forever(poll_interval=0.5) | ||
| - shutdown() | ||
| - handle_request() # if you do not use serve_forever() | ||
| - fileno() -> int # for selector | ||
| - handle_request() # if you don't use serve_forever() | ||
|
|
||
| Methods that must be overridden: | ||
|
|
||
| - get_request() -> request, client_address | ||
|
|
||
| Methods that may be overridden: | ||
|
|
||
| - server_bind() | ||
| - server_activate() | ||
| - get_request() -> request, client_address | ||
| - handle_timeout() | ||
| - verify_request(request, client_address) | ||
| - server_close() | ||
|
|
@@ -186,15 +187,11 @@ class BaseServer: | |
| instances: | ||
|
|
||
| - timeout | ||
| - address_family | ||
| - socket_type | ||
| - allow_reuse_address | ||
| - allow_reuse_port | ||
|
|
||
| Instance variables: | ||
|
|
||
| - server_address | ||
| - RequestHandlerClass | ||
| - socket | ||
|
|
||
| """ | ||
|
|
||
|
|
@@ -280,7 +277,9 @@ def handle_request(self): | |
| """ | ||
| # Support people who used socket.settimeout() to escape | ||
| # handle_request before self.timeout was available. | ||
| timeout = self.socket.gettimeout() | ||
| timeout = None | ||
| if hasattr(self, "socket"): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems a little odd. We assumed that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's based on going back to the original intent of making BaseServer and TCPServer separate classes. Originally, there was only TCPServer. BaseServer was created to abstract away from a socket-based interface to allow a more general idea of requests. The original commit from 2001 suggests the use case of a subclass of BaseServer which connects to a SQL database instead of a socket. The commit which added this part was later, in 2008, and I think just didn't realize that that was supposed to be part of the separation between the two. This is a minimal adjustment to ensure that existing uses aren't affected, but non-socket servers are possible again. |
||
| timeout = self.socket.gettimeout() | ||
| if timeout is None: | ||
| timeout = self.timeout | ||
| elif self.timeout is not None: | ||
|
|
@@ -325,6 +324,13 @@ def _handle_request_noblock(self): | |
| else: | ||
| self.shutdown_request(request) | ||
|
|
||
| def get_request(self): | ||
| """Get the request and client address from the socket. | ||
|
|
||
| Must be overridden by subclasses. | ||
| """ | ||
| raise NotImplementedError | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suspect this has already been brought up, but bring me up to speed--why do this instead of using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pure conservatism; I wasn't sure about adding abstract methods to a class that doesn't have any right now. I think it would be correct to do so, but this code pre-dates the existence of abstract methods and I didn't feel confident enough to pull that in. I'd be happy to do so if that's generally a safe thing to do. |
||
|
|
||
| def handle_timeout(self): | ||
| """Called if no new request arrives within self.timeout. | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| :meth:`socketserver.BaseServer.get_request` now raises ``NotImplementedError``. | ||
| It was previously absent and attempting to use it would generate an | ||
| ``AttributeError``. :class:`socketserver.BaseServer` no longer assumes that | ||
| all subclasses will add a ``socket`` attribute. Updated documentation to | ||
| clarify the differences between :class:`socketserver.BaseServer` and | ||
| :class:`socketserver.TCPServer`. |
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 seems more clear to me:
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.
I'm okay with that, but note that:
a) I didn't write this text, I just moved it to a new section
b) The current language is consistent with usage in the rest of the file. They should probably be all updated together if we're going to make this particular adjustment.