Opened 13 years ago
Last modified 6 years ago
#3711 enhancement new
Support Deferreds in Resource.render
Reported by: | esteve | Owned by: | hawkowl |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | web | Keywords: | |
Cc: | Thijs Triemstra, Michael Hudson-Doyle | Branch: |
deferreds-in-resrender-3711
branch-diff, diff-cov, branch-cov, buildbot |
Author: | esteve, washort |
Description (last modified by )
Twisted.web2 supports returning Deferreds from the Resource#renderHTTP method, Twisted.web should support this same behavior in Resource.render.
Related: #3621
Attachments (4)
Change History (34)
Changed 13 years ago by
Attachment: | deferred-render.patch added |
---|
comment:1 Changed 13 years ago by
Keywords: | moshez added |
---|---|
Owner: | jknight deleted |
comment:2 Changed 13 years ago by
Keywords: | review added; moshez removed |
---|
comment:3 Changed 13 years ago by
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
Attachment: | deferred-render-2.patch added |
---|
comment:4 Changed 13 years ago by
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
Attachment: | deferred-render-3.patch added |
---|
comment:5 Changed 13 years ago by
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
Author: | → esteve |
---|---|
Keywords: | review added |
Owner: | esteve deleted |
Thanks esteve, putting it up for review.
comment:7 Changed 13 years ago by
Author: | esteve → esteve, exarkun |
---|---|
Branch: | → branches/deferred-render-3711 |
(In [26597]) Branching to 'deferred-render-3711'
comment:9 Changed 13 years ago by
Author: | esteve, exarkun → 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:
- twisted/web/server.py
- The
render
method should have a docstring. Since you're modifying it, you get to add one. ;) - 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 :).
- 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.
- in
_cbRender
,types.StringType
should be replaced withstr
and probablyisinstance
should be used - in
_ebRender
, ther == UnsupportedMethod
check is redundant. trap won't let that code execute if the check is going to fail. Unless there's a subclass ofUnsupportedMethod
that should be handled differently? But I don't think there is. - in
_ebRender
, in the "HEAD" case, there is a second call to the resource'srender
. This call isn't handling a Deferred result. I think there's another ticket open about how this code is broken anyway, #3684.
- The
- twisted/web/test/test_web.py
SimpleDeferredResource
should have a docstring. It should probably also not usecallLater
, but what exactly it should use will depend on the next point...- 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
Owner: | changed from esteve to David Reid |
---|---|
Status: | new → assigned |
comment:12 Changed 12 years ago by
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
Cc: | Michael Hudson-Doyle added |
---|
comment:14 Changed 12 years ago by
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?
Changed 12 years ago by
Attachment: | deferred_render-3711-landreville.patch added |
---|
comment:16 Changed 12 years ago by
Keywords: | review added |
---|---|
Owner: | David Reid deleted |
Status: | assigned → 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:19 Changed 12 years ago by
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 12 years ago by
Description: | modified (diff) |
---|---|
Summary: | Support Deferreds in Resource#render → Support Deferreds in Resource.render |
comment:21 Changed 11 years ago by
Author: | esteve → esteve, washort |
---|---|
Branch: | branches/deferred-render-3711 → branches/deferred-render-3711-2 |
(In [31163]) Branching to "deferred-render-3711-2"
comment:24 Changed 11 years ago by
Keywords: | review added |
---|---|
Owner: | Jason McKellar deleted |
I believe I've addressed the remaining issues in the review and improved test coverage sufficiently. Ready for review.
comment:25 Changed 11 years ago by
Owner: | set to therve |
---|
comment:26 Changed 11 years ago by
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
?
- It would be nice to use
self._errorResource
consistently.
- There are 3 blank lines between
test_implicitHeadDeferred
andtest_badRenderResultDeferred
but only one betweentest_badRenderResultDeferred
andtest_renderError
.
- 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 11 years ago by
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: 29 Changed 11 years ago by
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 Changed 11 years ago by
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.
comment:30 Changed 6 years ago by
Branch: | branches/deferred-render-3711-2 → deferreds-in-resrender-3711 |
---|---|
Owner: | changed from washort to hawkowl |
I tried this on the plane. Lets see if the builders like it!
This patch should let Resource return a Deferred, while maintaining compatibility