Opened 6 years ago

Last modified 3 years ago

#3711 enhancement new

Support Deferreds in Resource.render

Reported by: esteve Owned by: washort
Priority: normal Milestone:
Component: web Keywords:
Cc: thijs, micahel@… Branch: branches/deferred-render-3711-2
(diff, github, buildbot, log)
Author: esteve, washort Launchpad Bug:

Description (last modified by glyph)

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

Related: #3621

Attachments (4)

deferred-render.patch (10.2 KB) - added by esteve 6 years ago.
This patch should let Resource return a Deferred, while maintaining compatibility
deferred-render-2.patch (12.3 KB) - added by esteve 6 years ago.
deferred-render-3.patch (13.6 KB) - added by esteve 6 years ago.
deferred_render-3711-landreville.patch (7.9 KB) - added by Landreville 4 years ago.

Download all attachments as: .zip

Change History (33)

Changed 6 years ago by esteve

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

comment:1 Changed 6 years ago by esteve

  • Keywords moshez added
  • Owner jknight deleted

comment:2 Changed 6 years ago by esteve

  • Keywords review added; moshez removed

comment:3 Changed 6 years ago by thijs

  • Cc thijs added
  • Keywords review removed
  • Owner set to esteve

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

Changed 6 years ago by esteve

comment:4 Changed 6 years ago by 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.

Changed 6 years ago by esteve

comment:5 Changed 6 years ago by 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.

comment:6 Changed 6 years ago by thijs

  • Author set to esteve
  • Keywords review added
  • Owner esteve deleted

Thanks esteve, putting it up for review.

comment:7 Changed 6 years ago by exarkun

  • Author changed from esteve to esteve, exarkun
  • Branch set to branches/deferred-render-3711

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

comment:8 Changed 6 years ago by exarkun

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

refs #3711

comment:9 Changed 6 years ago by exarkun

  • Author changed from esteve, exarkun to esteve
  • Keywords review removed
  • Owner set to esteve

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.

comment:10 Changed 5 years ago by dreid

  • Owner changed from esteve to dreid
  • Status changed from new to assigned

comment:11 Changed 5 years ago by exarkun

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

comment:12 Changed 4 years ago by 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.

comment:13 Changed 4 years ago by mwh

  • Cc micahel@… added

comment:14 Changed 4 years ago by 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?

comment:15 Changed 4 years ago by dreid

Absolutely not uncool. In fact very very very cool.

Changed 4 years ago by Landreville

comment:16 Changed 4 years ago by Landreville

  • Keywords review added
  • Owner dreid deleted
  • Status changed from assigned to new

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

comment:17 Changed 4 years ago by exarkun

Is this patch against trunk or the branch?

comment:18 Changed 4 years ago by Landreville

trunk

comment:19 Changed 4 years ago by exarkun

  • Keywords review removed
  • Owner set to Landreville

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.

comment:20 Changed 4 years ago by glyph

  • Description modified (diff)
  • Summary changed from Support Deferreds in Resource#render to Support Deferreds in Resource.render

comment:21 Changed 4 years ago by washort

  • Author changed from esteve to esteve, washort
  • Branch changed from branches/deferred-render-3711 to branches/deferred-render-3711-2

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

comment:22 Changed 4 years ago by washort

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

comment:23 Changed 4 years ago by washort

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

comment:24 Changed 4 years ago by washort

  • Keywords review added
  • Owner Landreville deleted

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

comment:25 Changed 4 years ago by therve

  • Owner set to therve

comment:26 Changed 4 years ago by therve

  • Keywords review removed
  • Owner changed from therve to washort

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?

  1. It would be nice to use self._errorResource consistently.
  1. There are 3 blank lines between test_implicitHeadDeferred and test_badRenderResultDeferred but only one between test_badRenderResultDeferred and test_renderError.
  1. 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!

comment:27 Changed 4 years ago by 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.

comment:28 follow-up: Changed 3 years ago by 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.

comment:29 in reply to: ↑ 28 Changed 3 years ago by 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.

Note: See TracTickets for help on using tickets.