Opened 13 years ago

Last modified 6 years ago

#3711 enhancement new

— at Support Deferreds in Resource.renderVersion 20

Reported by: esteve Owned by: Jason McKellar
Priority: normal Milestone:
Component: web Keywords:
Cc: Thijs Triemstra, Michael Hudson-Doyle Branch: branches/deferred-render-3711
branch-diff, diff-cov, branch-cov, buildbot
Author: esteve

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

Change History (24)

Changed 13 years ago by esteve

Attachment: deferred-render.patch added

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

comment:1 Changed 13 years ago by esteve

Keywords: moshez added
Owner: jknight deleted

comment:2 Changed 13 years ago by esteve

Keywords: review added; moshez removed

comment:3 Changed 13 years ago by Thijs Triemstra

Cc: Thijs Triemstra added
Keywords: review removed
Owner: set to esteve

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

Changed 13 years ago by esteve

Attachment: deferred-render-2.patch added

comment:4 Changed 13 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 13 years ago by esteve

Attachment: deferred-render-3.patch added

comment:5 Changed 13 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 13 years ago by Thijs Triemstra

Author: esteve
Keywords: review added
Owner: esteve deleted

Thanks esteve, putting it up for review.

comment:7 Changed 13 years ago by Jean-Paul Calderone

Author: esteveesteve, exarkun
Branch: branches/deferred-render-3711

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

comment:8 Changed 13 years ago by Jean-Paul Calderone

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

refs #3711

comment:9 Changed 13 years ago by Jean-Paul Calderone

Author: esteve, exarkunesteve
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 12 years ago by David Reid

Owner: changed from esteve to David Reid
Status: newassigned

comment:11 Changed 12 years ago by Jean-Paul Calderone

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

comment:12 Changed 12 years ago by Michael Hudson-Doyle

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 12 years ago by Michael Hudson-Doyle

Cc: Michael Hudson-Doyle added

comment:14 Changed 11 years ago by Jason McKellar

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 11 years ago by David Reid

Absolutely not uncool. In fact very very very cool.

Changed 11 years ago by Jason McKellar

comment:16 Changed 11 years ago by Jason McKellar

Keywords: review added
Owner: David Reid deleted
Status: assignednew

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 11 years ago by Jean-Paul Calderone

Is this patch against trunk or the branch?

comment:18 Changed 11 years ago by Jason McKellar

trunk

comment:19 Changed 11 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Jason McKellar

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 11 years ago by Glyph

Description: modified (diff)
Summary: Support Deferreds in Resource#renderSupport Deferreds in Resource.render
Note: See TracTickets for help on using tickets.