Opened 11 years ago

Closed 5 years ago

Last modified 4 years ago

#253 enhancement closed duplicate (duplicate)

Resource.render should allow returning a Deferred

Reported by: jknight Owned by:
Priority: normal Milestone:
Component: web Keywords:
Cc: itamarst, dp, jknight Branch:
Author: Launchpad Bug:

Description


Attachments (2)

render-deferred.patch.rtf (5.4 KB) - added by jknight 11 years ago.
render-deferred.patch (5.0 KB) - added by jknight 11 years ago.

Download all attachments as: .zip

Change History (22)

Changed 11 years ago by jknight

comment:1 Changed 11 years ago by jknight

This patch fixes the API for Resource.render to allow you to return a 
Deferred, as one would expect to be able to do. This is as opposed to 
the currently recommended method of returning NOT_DONE_YET and 
restructuring your code to call request.write(...). Of course, that should 
still work, too.

Changed 11 years ago by jknight

comment:2 Changed 11 years ago by jknight

bleargh, stupid RTF file.

comment:3 Changed 11 years ago by itamarst

There are a few places in the code that assume result of
render() is either NOT_DONE_YET or string. This is typically
done in wrapper resources that want to be transparent and
not alter request (e.g. my patch for improving
DeferredResource, or twisted.web.guard.) I suspect this
patch will break them.

comment:4 Changed 11 years ago by moshez

I've removed the [PATCH] indicator, since the current patch 
isn't complete -- like itamar said, at least patch 
twisted.web.guard

comment:5 Changed 11 years ago by itamarst

It's not just web.guard, it's other code in my own projects
and possibly others. I'm not sure if there is a solution :(
You can do stuff like having a reallyRender that render
calls and can return Deferreds which are converted to
NOT_DONE_YET, or... I dunno. Make __eq__ and friends for
NOT_DONE_YET do some magic for deferreds?

comment:6 Changed 11 years ago by jknight

Surely t.w.guard should be fixed (sorry didn't notice that problem). 
However, I'm not sure how much effort really needs to be put into being 
totally compatible with user code. Here is my proposed solution:
1) For the next X amount of time, do not return a deferred from render() 
in any code in the twisted framework (unless it's just passing it through 
a user's render()).
2) Make all code in twisted framework able to deal with user code 
returning a deferred.

With this solution, breakage can only occur if a user's code returns a 
deferred from render, *and* the user's code isn't able to deal with that. 
Nothing that currently works would break.

I think that's a reasonable compromise.

comment:7 Changed 11 years ago by itamarst

That seems like a reasonable solution, yes. Some sort of
utility code for wrapper resources so they don't have to
write the same code each time would also make it easier to
transition - "oh, just use this if you're doing that sort of
thing".

comment:8 Changed 11 years ago by moshez

Can this be downgraded from "bug" to "feature"?
itamar?

comment:9 Changed 11 years ago by glyph

Somehow I feel as though 'bug' is the right classification, but at the same time
it's changing an existing interface which is ANCIENT in Twisted's time-scale
(which has problems as indicated).

jknight, can you update the patch to provide the policy that you suggest?  It
sounds good.

I'm assigning this to dp to make sure someone is in charge of following up and
getting this committed eventually.  I'd like it to be in 1.1 so that we can end
end "X amount of time" when we hit 1.2 and drop the other vestiges of ugly
backwards compatibility.

comment:10 Changed 11 years ago by jknight

I've tried to decipher the widget code in t.w.guard. The simple solution of just returning the 
subresource's render(..) isn't possible because I guess the return value of reallyRender needs to be 
widgets.FORGET_IT so that the first time after it authenticates you, it doesn't redisplay the login 
form.

So, anyhow -- I *think* guard can be fixed by the following. It is somewhat odd as it calls back 
into the request's render method which you don't generally do, but it should work because the first 
render call aborted with NOT_DONE_YET and no headers have been sent yet. The test doesn't run 
because DummyRequest doesn't implement render. I don't see a simple example of t.w.guard use
and have no idea how all this set of authorizer/etc stuff works. 

So, this is untested but probably works. :)

@@ -210,10 +207,7 @@
     def reallyRender(self, request):
         # it's authenticated already...
         res = self.res.getChildForRequest(request)
-        val = res.render(request)
-        if val != NOT_DONE_YET:
-            request.write(val)
-            request.finish()
+        request.render(res)

comment:11 Changed 11 years ago by itamarst

This won't work for the case returns a string. Or for the case it returns a
Deferred ;)

comment:12 Changed 11 years ago by jknight

Hm? Explain? request.render doesn't return anything.

comment:13 Changed 11 years ago by itamarst

Good point! I thought you did resource.render. I'll check the request.render. If
it *doesn't* do what we want, we ought to add a method to Request that does :)

comment:14 Changed 11 years ago by moshez

Re: testing.
The latest tests in test_web.py show a way to use real requests, not dummy
ones.

comment:15 Changed 11 years ago by radix

Now that I've implemented that render_* change, allowing resources to return
deferreds from render_* should be easy: put the code to manage it in base
Resource.render.

comment:16 Changed 10 years ago by radix

does nevow/twisted.web2 do this already?

comment:17 Changed 10 years ago by jknight

This bug is irrelevant now, people should use Nevow, or web2 whenever it's done to get this 
functionality. Closing wontfix.

comment:18 Changed 5 years ago by ghazel

  • Resolution fixed deleted
  • Status changed from closed to reopened

twisted.web is alive and well. People very recently have been wondering why this wasn't ever improved:
http://jcalderone.livejournal.com/51109.html?thread=116389#t116389
http://jcalderone.livejournal.com/50226.html?thread=113970#t113970

Perhaps we should revisit this issue?

comment:19 Changed 5 years ago by exarkun

  • Resolution set to duplicate
  • Status changed from reopened to closed

#3711 was a duplicate of this. Since a bunch of work has been started there, we'll keep this ticket closed as the duplicate though (whereas normally I would close the higher numbered ticket as the duplicate).

comment:20 Changed 4 years ago by <automation>

  • Owner dp deleted
Note: See TracTickets for help on using tickets.