Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #2268 (closed defect: fixed)

Opened 10 years ago

Last modified 10 years ago

override_template with concurrency and functionality issues

Reported by: droggisch Owned by:
Priority: highest Milestone: 2.0rc1
Component: TurboGears Version: 2.0b7
Severity: normal Keywords:
Cc:

Description

When using override template, this manipulates the decoration-object of the controller. First of all, this is permanent, so a subsequent request to the same controller would also deliver the overridden template - certainly, that's not desired.

Additionally, this exposes a concurrency issue as well, as the information is global.

The patch attached to ticket #2267 fixes this.

Change History

comment:1 Changed 10 years ago by mramm

Well the controller is re-instantiated on every request so I don't belive that subsequent requests will be effected. And I highly recommend actually returning a string in most of the cases where you want to override the template.

If there's a test case that shows this happening, it moves it from being an useful cleanup to a important bugfix.

comment:2 Changed 10 years ago by droggisch

Well, we had the problem with our TG2 (b6). Our controllers looked like this before the fix:

@expose()
def foo(self):
   override_template("the.template.I.usually.want")
   if some_condition:
     override_template("other.template")

And since when are the controllers instantiated every time? The nested controller idiom is

class Root():

sub = SubController?()

so even if you'd instantiate Root, SubController? would not be re-instantiated.

But even if they were, the decoration is part of the unbound method, isn't it?

comment:3 Changed 10 years ago by droggisch

I just re-checked, and can confirm that it's really a critical bug.

You can test that yourself if you just apply the test included - it fails unless the full patch is applied.

I had to re-create the patch btw, it was somehow broken.

comment:4 Changed 10 years ago by mramm

  • Priority changed from normal to highest

Ok, I notice you have tests in your patch. And it looks good to me. Feel free to get the karma points and commit the patch to trunk ;)

Or let me know, and I'll do it.

Thanks for tracking this down, testing it, and supplying a patch.

comment:5 Changed 10 years ago by droggisch

  • Status changed from new to closed
  • Resolution set to fixed

Applied in r6494. karma kaching

Note: See TracTickets for help on using tickets.