gh-127209: Clarify and clean up the separation between BaseServer and TCPServer

[tungol]

This turned out to mostly be a documentation change. I updated the documentation and docstrings to clarify what's from BaseServer and what's added by TCPServer. For code changes, I added a stub method for BaseServer.get_request which just raises NotImplementedError.

While examining the history of the module, it's clear to me that BaseServer was intended to not reference the socket used by TCPServer and it's subclasses. You can see the original commit which created BaseServer here: 90cb906

The use of self.socket in BaseServer.handle_request was introduced much later, and seems to me like it broke the expected separation. I added the private method BaseSever._get_timeout so that TCPServer can handle socket.settimeout() without requiring alternate implementations of BaseServer to have a self.socket attribute.

Unlike get_request, nothing in BaseServer uses fileno or server_bind, so I documented those two as being introduced by TCPServer rather than create a stub method for them.

I think it's also worth noting that TCPServer also adds a allow_reuse_port attribute which is not currently documented. I wasn't sure if there was a reason for that, so I didn't try to add it.

As suggested by @picnixz in the issue, this MR leaves the question of whether BaseServer.get_request should be marked as abstract for another time. It would make sense to do so, but I haven't checked any possible performance implications, so this is the conservative approach.

• Issue: Should socketserver.BaseServer have abstract methods? #127209

---

📚 Documentation preview 📚: https://cpython-previews--127976.org.readthedocs.build/en/127976/library/socketserver.html#module-socketserver

[picnixz]

For code changes, I added a stub method for BaseServer.get_request which just raises NotImplementedError.

Question: can this break existing code now that we raise a NotImplementedError. Sorry, I didn't think of this case when I suggested this... And while I like having a stricter runtime checks, I would very much prefer that we don't break any existing code, or find a way not to break it =/ (maybe a warning?)

[tungol]

I don't think there's much risk. Any code that will hit that function instead of TCPServer.get_request is getting AttributeError: 'BaseServer' object has no attribute 'get_request' right now, or something similar.

[picnixz]

Although I understand that a BaseServer is socket-less, if possible, I'd leave out the updates on the timeout and only raise NotImplementedError in get_request(). Are there projects which directly inherit from BaseServer and define their own socket property, thereby "implicitly" broken by the change?

If you are willing to how timeout is handled, this requires a true What's New entry to indicate that we no more use self.socket.gettimeout().

Finally, I think we should indicate that get_request raises an exception in a What's New entry because it's still a user-facing change (and it doesn't hurt to remind users) (and that before it was an AttributeError being raised). The rationale is that people could catch AttributeError but not NotImplementedError (although I agree that this probably not the best coding practice; better catch both if you're worried about an object missing an interface to implement)

[picnixz]

Wouldn't this affect users that directly inherit from BaseServer instead?

[tungol]

The new version I just pushed leaves the existing timeout logic in place, but uses hasattr to make it safe for any subclasses that don't have a socket attribute. I can put it back to the way it was if you think that's still too much, but this should be less obtrusive while still increasing the range of possibilities for users of the base class.

[tungol]

Maybe the NEWS entry should be moved out of the documentation category at this point? [EDIT] I moved it to Library

[ZeroIntensity]

I didn't really look too much at the issue, and just did a blind review; if I ask stupid questions or bring things up that have already been discussed, forgive me!

[ZeroIntensity]

This seems more clear to me:

Suggested change

	May be overridden.
	May be overridden by a subclass.

[tungol]

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.

[ZeroIntensity]

The only values, right?

[tungol]

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 "are the most common values" would work to strengthen the statement.

[ZeroIntensity]

This seems a little odd. We assumed that the socket attribute was present before--why not now?

[tungol]

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.

[ZeroIntensity]

I suspect this has already been brought up, but bring me up to speed--why do this instead of using abc.abstractmethod?

[tungol]

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.