Support Deferreds in Resource.render

[esteve]

Twisted.web2 supports returning Deferreds from the Resource#renderHTTP method, Twisted.web should support this same behavior in Resource.render.

Related: #3621

[esteve]

This patch should let Resource return a Deferred, while maintaining compatibility

[thijs]

We'll need unit tests for any changes, can you create those as well?

[esteve]

thijs: Here it is, I wrote it on the plane but forgot to upload it. Let me know if you need a more exhaustive test.

[esteve]

I've updated the docs for IResource#render, but it probably needs a review. BTW, my patch makes trial complain about a lot of deprecated uses of NOT_DONE_YET, I can fix those, if you think deprecating NOT_DONE_YET is ok.

[thijs]

Thanks esteve, putting it up for review.

[exarkun]

(In [26597]) Branching to 'deferred-render-3711'

[exarkun]

(In [26598]) Apply deferred-render-3.patch

refs #3711

[exarkun]

Thanks for the patch Esteve! I think you're on the right track. Here are a few things, mostly minor:

1. twisted/web/server.py
   1. The render method should have a docstring. Since you're modifying it, you get to add one. ;)
   2. The existing code checks NOT_DONE_YET in two different ways - by identity and by equality. Probably the new code should make this consistent, to avoid any really confusing cases where the two checks end up disagreeing (not sure if this is actually possible on current versions of CPython, but let's not risk it :).
   3. I think it's probably too early to deprecate NOT_DONE_YET. This is a really old API with lots of users. We should have a good replacement for it for a while, with docs helping people migrate, before we start yelling at them for not switching.
   4. in _cbRender, types.StringType should be replaced with str and probably isinstance should be used
   5. in _ebRender, the r == UnsupportedMethod check is redundant. trap won't let that code execute if the check is going to fail. Unless there's a subclass of UnsupportedMethod that should be handled differently? But I don't think there is.
   6. in _ebRender, in the "HEAD" case, there is a second call to the resource's render. This call isn't handling a Deferred result. I think there's another ticket open about how this code is broken anyway, #3684.
2. twisted/web/test/test_web.py
   1. SimpleDeferredResource should have a docstring. It should probably also not use callLater, but what exactly it should use will depend on the next point...
   2. It doesn't seem as though anything tests the new Deferred support. test_simplestSiteDeferred only does "resource traversal". It doesn't invoke any render method. So more tests are needed, I think.

The only really significant problem I see is the missing test coverage. I suspect there are a few lurking corner cases that will need to be carefully tested to make sure they're working right and keep working right.

[exarkun]

This was a duplicate of #253, by the way.

[mwh]

I think I've addressed most of the review comments for server.py now, apart from the one to do with HEAD handling (my brain hurts when I look at that code).

More tests still needed.

[Landreville]

This has been sitting here for three months now. Would it be uncool if I kinda hijacked this ticket and posted a patch and test case that satisfies the problems in exarkuns review?

[dreid]

Absolutely not uncool. In fact very very very cool.

[Landreville]

Ok I've attached a patch. I'm unsure of the test I added. It tests it alright, but it takes three seconds because I use deferLater to return a deferred from the resource and also deferLater to check if the request wrote the correct output.

The patch I submitted is deferred_render-3711-landreville.patch

[exarkun]

Is this patch against trunk or the branch?

[Landreville]

trunk

[exarkun]

I think there was a misunderstanding when you (Landreville) took over this ticket. The latest patch seems to be a fresh start at implementing this feature, rather than building on the work in the branch linked in the ticket description (branches/deferred-render-3711). Starting fresh can be a good thing, but only with a good reason. The main reason not to start fresh is partially academic in this case at this point - it's almost always more work (both for authors and reviews) than finishing someone else's work. Still, without a good reason, I'm not sure I want to spend the time to review this latest patch carefully when superficially looks like it's mostly a rehash of the work in branches/deferred-render-3711. Ideally, a patch to finish this ticket would be made against the branch and just make the changes necessary to respond to the review comments on the ticket.

If you take a look at the unit tests in the branch, you'll see how they avoid a delay.

I hope this isn't too discouraging. The help is really appreciated. Sorry the way to proceed on this ticket wasn't more clear.

[washort]

(In [31163]) Branching to "deferred-render-3711-2"

[washort]

(In [31182]) Branching to "deferred-render-3711-2".

[washort]

(In [31200]) Branching to 'deferred-render-3711-2'

[washort]

I believe I've addressed the remaining issues in the review and improved test coverage sufficiently. Ready for review.

[therve]

1.

+            if body is NOT_DONE_YET:
+                # In a future release of Twisted, we should issue a
+                # DeprecationWarning here.
+                return

Please open a ticket and refer to the ticket number in the comment.

After thinking about it, maybe not? Aren't there use cases not covered by returning a Deferred?

2.

        self.method = "GET"                                                                    
        self._inFakeHead = True                                                                
        body = resrc.render(self)

It seems there may be a problem here if the render returns a Deferred?

3. It would be nice to use self._errorResource consistently.

4. There are 3 blank lines between test_implicitHeadDeferred and test_badRenderResultDeferred but only one between test_badRenderResultDeferred and test_renderError.

5. We should update the documentation, although I suppose it could be done in a future ticket. Please open it if you don't do it here.

Thanks!

[lvh]

There's a problem with this ticket.

IResource.render says it can return a string or NOT_DONE_YET. Deferreds are neither. As a result, this would break the interface, and that would be bad.

The way to fix this is a new interface. See #5152.

[itamar]

Adding another option to render() does not break the interface, it extends it. It's possible that extending the interface will break things, e.g. wrapper code that currently calls render() without expecting a Deferred. But extending the interface is not automatic grounds for rejection.

[glyph]

Replying to itamar:

Adding another option to render() does not break the interface, it extends it. It's possible that extending the interface will break things, e.g. wrapper code that currently calls render() without expecting a Deferred.

Perhaps "broken" is too vague a term (as is "adding another option", since Python functions don't have options, they have arguments and they have return values). This does constitute an incompatible modification of the interface. For example, if you have an interface that says it returns an int but then the interface were changed to be int or str or None, callers of that interface would be broken by that change; they would raise exceptions where they previously worked correctly. The same is true of int versus "Deferred firing with int".

But extending the interface is not automatic grounds for rejection.

I would say that extending it in this manner is, as it's a violation of the compatibility policy. Of course, any such policy violation may be challenged and discussed on the mailing list, and if consensus can be achieved that nobody actually cares about the change and it won't break any running software, we can apply it as an exception to the policy.

It might be technically compatible to change the documentation without actually changing any implementations or any callers of the interface, since the goal behind the compatibility policy is to avoid causing software to break, and a changed interface docstring wouldn't cause failures anywhere (not even in verifyObject, since it can't tell what return type we're talking about). But that strikes me as rather pointless.