Misleading docstring in RequestHandler.prepare()

[pawciobiel]

I think RequestHandler.prepare() docstring is misleading because it doesn't mention that the method has to be in SUPPORTED_METHODS - otherwise prepare() won't be called.

I suggest we change:

        Override this method to perform common initialization regardless of the request method.

to:

        Override this method to perform common initialization for request method.                                                                                     
        If the request method is not in ``SUPPORTED_METHODS`` this will not be called. 

[pawciobiel]

#3431

[bdarnell]

It's true that the docs don't match the behavior here, but it's also worth looking at whether the current behavior is what we want. Why exactly was this an issue for you? Was it because your on_finish was expecting prepare to have been called? That's a reasonable expectation; I've said before (in #2711 and #517) that the intended rule was "if prepare() is called, then exactly one of on_finish() or on_connection_close() will be called".

There are some exceptions to that rule today. SUPPORTED_METHODS is one, but errors from decode_argument and check_xsrf_cookie do the same thing. (and in the other direction, the fact that some errors call on_connection_close instead of on_finish can be confusing). Other errors short-circuit the process earlier, often in subtle ways: decoding the URL query string happens in two phases (first as bytes, and then as characters), and an error in the second phase will call on_error while an error in the first phase will not.

So some things we could do are:

• Document when prepare() will and will not be called. I think that giving the full details of which errors are handled which way is too much minutiae and I'd rather just give a weaker guarantee like "if an HTTP method is called, prepare will be called before it. On errors, prepare is not guaranteed to be called".
• Call prepare in more cases, for example even when the SUPPORTED_METHODS check fails. This seems kind of silly to me since we know at this point that the request is going to fail, so why go through a prepare/on_finish cycle?
• Document when on_finish will be called, and that it is sometimes called when prepare was not.
• Call on_finish in fewer cases, so that it is called if and only if prepare has been called.

My inclination is to not provide too many specifics about prepare and errors, and to only call on_finish (and on_connection_close) if prepare was called, but I'd like to hear more about what kind of problem you encountered that prompted this issue.

[pawciobiel]

Hello @bdarnell, thank you for prompt reply.

Was it because your on_finish was expecting prepare to have been called?

yes , precisely that. I have some additional metrics setup in prepare and I use it and do some cleanup in on_finish.

The code I work on has been written a while ago and changed by a number of people along the way and as a result the OO structure / inheritance of handlers is in a bad shape. Occasionally I need to restrict access to request methods in a child Handler. It's convenient to use SUPPORTED_METHODS for that.

Document when prepare() will and will not be called. I think that giving the full details of which errors are handled which way is too much minutiae and I'd rather just give a weaker guarantee like "if an HTTP method is called, prepare will be called before it. On errors, prepare is not guaranteed to be called".

I agree, documenting all the side effects is just impossible...

Call prepare in more cases, for example even when the SUPPORTED_METHODS check fails. This seems kind of silly to me since we know at this point that the request is going to fail, so why go through a prepare/on_finish cycle?

sure, it makes no sense

Call on_finish in fewer cases, so that it is called if and only if prepare has been called.

I know that I have code that really on on_finish to clean up things that are setup in initialize additionally to cleanup things that are setup in prepare. And in the past I had problems with memory leaks if things weren't clean up in on_finish (or on_finish was not called).

Another option could be to remove prepare completely (I guess doing all the setup in initialize may not be ideal for some cases...)
or alternatively add another method - an antagonist to prepare (Less code is usually less problems and I don't seem to like any of antonyms proposed by thesaurus... ;-))

Thanks a lot for the clarification and all of your work on tornado!

[bdarnell]

I know that I have code that really on on_finish to clean up things that are setup in initialize additionally to cleanup things that are setup in prepare.

OK. Cleanup from initialize() is not something we have a solution to right now. initialize() is roughly equivalent to __init__(), so cleanup (if any) is handled by __del__() (with all its drawbacks). (Why does the initialize method exist? Just because it's annoying to remember the application, request arguments to __init__ and to be sure to call super(). I've been tempted to define def initialize(self, **kwargs): [self.setattr(k, v) for (k, v) in kwargs.items] and then discourage overriding it).

Another option could be to remove prepare completely (I guess doing all the setup in initialize may not be ideal for some cases...)

initialize() is not a substitute for prepare() because it happens too soon, before we can cleanly send a response. prepare can return a 403 error (for example), but any errors in initialize just get a generic error response. We're closer to removing initialize than prepare, I think (although I wouldn't actually remove either).

Our most complete documentation on this is at https://www.tornadoweb.org/en/stable/guide/structure.html#overriding-requesthandler-methods, which is probably not the best location, and it's also incomplete (this is what #2711 is about). But I think the best approach is to ensure that this page is complete and increase its visibility with links from all the relevant methods.

[pawciobiel]

ok, thanks again for the clarification.