In SocketServer, why not passing a factory instance for the RequestHandlerClass instead of the class itself?

[MaharishiCanada]

BPO	29947
Nosy	@ericvsmith, @bitdancer, @vadmium, @MaharishiCanada
PRs	bpo-29447: tempfile: Add pathlike object support for dir argument #1496
Files	Issue29947_for_discussion_04.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2017-03-30.12:08:53.784>
labels = ['3.7', 'type-bug', 'library', 'docs']
title = 'In SocketServer, why not passing a factory instance for the RequestHandlerClass  instead of the class itself?'
updated_at = <Date 2017-05-08.04:16:49.390>
user = 'https://github.com/MaharishiCanada'

bugs.python.org fields:

activity = <Date 2017-05-08.04:16:49.390>
actor = 'louielu'
assignee = 'docs@python'
closed = False
closed_date = None
closer = None
components = ['Documentation', 'Library (Lib)']
creation = <Date 2017-03-30.12:08:53.784>
creator = 'dominic108'
dependencies = []
files = ['46781']
hgrepos = []
issue_num = 29947
keywords = ['patch']
message_count = 20.0
messages = ['290840', '290855', '290886', '290887', '290892', '290952', '290963', '290976', '290996', '291002', '291004', '291005', '291006', '291024', '291026', '291036', '291048', '291056', '291142', '291181']
nosy_count = 5.0
nosy_names = ['eric.smith', 'r.david.murray', 'docs@python', 'martin.panter', 'dominic108']
pr_nums = ['1496']
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue29947'
versions = ['Python 3.6', 'Python 3.7']

[MaharishiCanada]

I am just curious to know if someone considered the idea of passing a factory instance that returns RequestHandlerClass instances instead of directly passing the class? It may affect existing handlers that read non local variables, but there should be a way to make the factory optional. The purpose is only aesthetic and a better organization of the code. I find it awkward to have to subclass the server every time that we have an handler that needs special objects, a database connection, a socket connection to another party, etc. The server class should have a single purpose: accept a request and pass it to an handler. We should only need to subclass a server when we need to do that in a different way : TCP vs UDP, Unix Vs INET, etc. The usage is simpler and more natural. Instead of subclassing the server, we create a factory for the handler.

[ericvsmith]

Just a guess, but it's because this code was written at a time when subclassing was the preferred way to extend code. Now we know better :), but it's not possible to change history, unfortunately. Lots of code depends on the current behavior.

If you find a way to make a factory function work, while preserving backward comparability, then we can re-open this issue and consider the enhancement request then.

[MaharishiCanada]

One way to make the factory optional is to offer a MixIn. I attached a file with a FactoryMixIn. It's just that I find it awkward that the proposed approach is to pass the extra parameters in a subclassed server. More modern approaches should also be offered.

[MaharishiCanada]

Finally, I looked around and people just use the server to pass any extra parameter. I do find it awkward, but it works and it is simple, in fact simpler than having to define a factory object. I don't close it, because I will be happy to see another opinion. I would use this factory approach for myself, because I am just not comfortable to add an attribute to a server that would not even look at it, but maybe that it just me.

[MaharishiCanada]

On the other hand, it occurs to me that this seems way more flexible than passing the object through the server, because you share the factory with the server, not only the object. This means that you could even change the type of the handler while the server is running !

[bitdancer]

I think I'm missing something here. What prevents one from passing a factory function as the RequestHandlerClass argument? In Python, a class name *is* a factory function for class instances.

[MaharishiCanada]

I am a bit ashamed that I missed that. Still, the intent in the current code, the name of the parameter, the examples, etc. is that we pass the handler class. This is more than its __init__ function and less than a generic factory method. An important difference, which could become important, is that with a factory the handler type could depend on the request. As pointed out by Eric, passing the class was perhaps the intent in the early days. Now, perhaps many use it differently and pass a factory method, not a class, but it still appears as a hack that does not respect the intent. One could legitimately worry that this hack will not be supported in future versions, because it is not documented.

[MaharishiCanada]

Perhaps I should raise a separate issue, but it is related, because the current code "requires" that we define an handler class with setup(), handle() and finish() in its API. If you look at the actual code, there is no such requirement. We only have to pass a factory method that receives three arguments, creates a new handler instance and starts it. The handler does not have to offer any API - you don't even have to subclass the RequestHandlerBase class. I still say that it "requires" it, because it is part of the documentation and, if you don't, you could be in trouble in future versions. However, this requirement is clearly annoying, because if you have an handler for another server, you need to re-factorize the code to provide this API, which is not even used. I am sure there is no such requirement, but it's not clear at first.

This is clearly not just a documentation issue. The intent in the code is the concern of the designers of the code. David and I assume that the intent is that we can provide any arbitrary factory method that receives the three arguments, creates and starts the handler, but we can be mistaken. In fact, to me this looks as a hack, especially given the parameter name, which says that we must pass the handler class. An handler class is indeed a factory method, but it is a very specific one, not the one that we want to pass if we must include extra parameters.

[MaharishiCanada]

To sum up, David clarified that we can in fact easily pass an arbitrary factory method that creates and starts a request handler, instead of a request handler class with setup, handle and finish in its API. This could indeed be a valid reason to consider this issue resolved, but I would like to be sure that it is really a normal and supported use of the code. The only place where I can ask this is here. If I ask in StackOverflow, etc. I would get all kind of opinions and it would not be useful. To be honest, looking at the parameter name, the documentation, the examples, etc. it does not look at all a normal and supported use of the code, but who am I to decide that? If it turns out to be a normal and supported use of the code, then I will create a different issue only about the documentation, because it's not clear at all.

[bitdancer]

Well, we could document it as a factory argument, and explain that the argument name is an historical artifact.

[bitdancer]

Given how old socket server is, and that it doesn't actually require that API, we should probably just reword the documentation so that it is clear that RequestHandlerClass is the default Handler implementation (and that you can work with setup/handle/finish if you want to subclass it), and document what the *actual* requirements on the Handler are.

We are unlikely to change socketserver's actual API at this point.

I don't think there's any need for a new issue, let's just make this a documentation issue. The original title still even applies :) We will need signoff from someone with more direct interest in the socketserver module than me, but given that the "future" for the stuff socketserver handles is asyncio, I suspect the only objection we might get is "it's not worth it". But if someone wants to do the work, *I* don't see a reason to reject it.

[MaharishiCanada]

The reason why I feel we should not make it immediately a documentation issue is that I don't know how a person working on documentation could proceed ahead without a clear go ahead signal from developers. In that sense, having a documentation that says that the variable name does not reflect its intent seems tricky. It's very easy to change a variable name. It cannot possibly break the code, except for a collision, but they are easy to avoid. If we don't have the interest of developers, not even for a simple renaming, I don't see a go ahead signal in this.

[MaharishiCanada]

Oops, I did not realize that David was one of the developers. Well, may be this needs the attention of more than one developer.

[vadmium]

By “factory instance”, I presume you just mean a function (or class or method) that returns an appropriate object when called. (I think these are normally called “factory functions”. Instances are objects, the things that class constructors and factories return.)

What is being proposed? The best idea that I understand is hinted in <https://bugs.python.org/issue29947#msg290976\>. That is, to document and allow any function (not just a class or factory) to be passed as (or instead of) the RequestHandlerClass parameter.

This is a significant change to the documentation and API, but I would support it, and I think it should already be supported by the implementation.

The question of renaming the RequestHandlerClass parameter is a harder decision IMO. I think it would be easier to make the documentation clear that it is a misnomer and does not have to be a class. But in the long term, renaming the parameter seems nicer.

If it was “renamed”, we would first have to add a competing parameter, keep the old parameter around, consider adding deprecation warnings in 2020 when Python 2 is dead, document both parameters, decide if anything should happen when both parameters are passed, etc. Much trickier, but possible. This renaming process is similar to “base64.encodestring” vs “encodebytes”, but more involved. I can’t think of an example where a function or constructor parameter was renamed like this.

[MaharishiCanada]

I did not think very far when said that renaming the parameter could not possibly break the code ! Oh well ! But, renaming the parameter was not important in itself. It was to make the situation clearer and easier for those who write the documentation. Martin mentioned that it is a big change for the API. This is what I had in mind. And yes, I should have used 'factory function', not 'factory instance'. Oh well ! I am glad things are working and that we will move ahead to resolve this issue.

[bitdancer]

Yes, the difficulty in renaming the parameter was why I suggested a doc change only. I'm not sure it it is worth it to go through a deprecation cycle for socketserver to change the name, though it certainly would be nice. Martin and I could make that decision, but it would be better to get input from other devs. And, if we make this the documentation issue, we should open a separate issue for the parameter rename.

[MaharishiCanada]

I started to look at the documentation to see what would need to be changed, assuming that we agree for a change in the API. Just for the purpose of this discussion, I created a patch that only change the comments in socketserver.py.

[MaharishiCanada]

The key point, IMHO, is that the BaseRequestHandler class is just provided as an option and its API (setup, handle and finish) is ignored by the code that we support.

Some applications may have used the API, but these are details in applications. Simply, we keep BaseRequestHandler as it is so that we do not break these applications.

So, yes, we should keep supporting the API, but we do not expect this API on our side. The latter is the key point.

[MaharishiCanada]

I simplified the patch. Using a class as a factory function is the simplest case, so I took advantage of this. I also give one way to pass extra parameters to the handler in the factory function, because it's the main reason why we cannot always use a class.

[MaharishiCanada]

An improved version of the patch, I hope. I will remove the old patch, because it's really does not help to see the old versions.