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 #2415 (closed defect: fixed)

Opened 9 years ago

Last modified 9 years ago

turbokid reload templates problems

Reported by: xaka Owned by: chrisz
Priority: normal Milestone: 1.0.x bugfix
Component: TurboKid Version: 1.0.9
Severity: normal Keywords:
Cc:

Description (last modified by Chris Arndt) (diff)

I have found a few situations where template reloading mechanism is working wrong and produces an error.

Example of situation 1:

  • Templates: A, B, C
  • Extends: B extends A, C extends A, i.e. B and C depend on A

To reproduce the error:

  • load page with template B
  • then load page with template C
  • then again load page with template B --> a NoneType exception will be raised.

What's wrong:

  • When C sees that A was changed, it delete A's module from sys.modules.
  • Then B loads and it tries to get A's module info from sys.modules, but it doesn't exist anymore and a KeyError exception occurrs.
  • As a result the load_template method fails and we get an exception.

Example of situation 2:

  • Templates: A, B, C
  • Extends: C extends B, B extends A, i.e. A -> B -> C

To reproduce the error:

  • load page with template C
  • then touch template A
  • then load page with template C again. An exception will raised.

What's wrong:

  • When loading module C the caching max mtime for each template depends on it bases.
  • Let's assume that A has max mtime. mtime<A> = t1, mtime<B> = mtime<A>, mtime<C> = mtime<A>.
  • Next we load template B. In this process we redefine mtimes of some base templates which stored max mtime. Now mtime<B> != mtime<A> --> load_template method fails again.

I've attach patch which fix all this bugs.

Attachments

02-reload-templates-fix.patch Download (2.2 KB) - added by xaka 9 years ago.
turbokid_test.zip Download (1.5 KB) - added by chrisz 9 years ago.
Demo of second problem
trunk.zip Download (1.3 KB) - added by xaka 9 years ago.

Change History

Changed 9 years ago by xaka

comment:1 Changed 9 years ago by Chris Arndt

  • Description modified (diff)

comment:2 Changed 9 years ago by Chris Arndt

Bug description, analysis and patch looks sound to me. If you can provide tests, I will apply the patch immediately. Otherwise, I (or another maintainer) will do it when I have time.

comment:3 Changed 9 years ago by chrisz

I can reproduce the second problems, and the first if I add the following steps before loading the page with template B again:

  • touch template A and change template C so that it has an error (e.g. not well-formed)
  • load template C again (displaying the error)

This is because by loading a correct template C the template A will be reloaded again.

The first fix in the patch seems to be ok. The second fix does not seem to be correct and will not fix the second problem.

The actual problem is that if you have the chain A -> B -> C and C changes, then you must reload not only A and C (as it is currently done), but also B, because after removing and reloading C, B cannot find the old C any more. I fixed this by always reloading all bases if any of them changes. This may be sometimes overkill, but I don't think it's worthwile to elaborate the exact dependency chain.

Tests for the problem above need to create files and provoke mtime changes on the file system, or create appropriate mock objects. I don't think it's worth that effort, since Kid is outdated anyway, so we should not spend much energy on TurboKid, and TurboKid never had any unit tests anyway. I have just checked that the two problems above are solved by my patch, all the TG 1.0 tests pass, and I have added some basic tests for TurboKid now which all pass. The changes are in r6984 and r6985.

Let me know what you think - if your're ok, I will create a new TurboKid release.

comment:4 follow-up: ↓ 5 Changed 9 years ago by xaka

How B can to depend on C in "A -> B -> C" chain? Reverse dependency, i.e. base class depend on child? I'm little confused.

comment:5 in reply to: ↑ 4 Changed 9 years ago by chrisz

Sorry, I interchanged A and C in that sentence. It should read:

"The actual problem is that if you have the chain A -> B -> C and A changes, then you must reload not only A and C (as it is currently done), but also B, because after removing and reloading A, B cannot find the old A any more."

comment:6 Changed 9 years ago by xaka

Yeap, you are right and i've said about this i description (may be not so clean). My patch fix that by one line "ct.setdefault(module, mtime)" - B will have mtime of A, C will have mtime of B. As result mtime(C) == mtime(B) == mtime(A). When you touched A and will try to use B or C - it automatically reload all self bases.

Changed 9 years ago by chrisz

Demo of second problem

comment:7 Changed 9 years ago by chrisz

xaka, I still don't see how the ct.setdefault(module, mtime) can fix this. Find attached a demo for your situation 2 problem. It has three templates a.kid, b.kid, c.kid. It loads c.kid, changes a.kid and reloads c.kid. This fails because the b module cannot find the a module any more. However, with your patch, the problem is the same.

comment:8 Changed 9 years ago by xaka

I had write additional tests for turbokid's trunk branch (include failed tests which helps to fix described issue), but can't attach the file:

Submission rejected as potential spam (Maximum number of external links per post exceeded)

comment:9 Changed 9 years ago by Chris Arndt

I have marked your submissions as ham (good) in the spam filter. Can you try to attach the file again, please?

Changed 9 years ago by xaka

comment:10 Changed 9 years ago by xaka

Added only after trunk.diff was packed into zip archive. Heh.

To apply patch:

cd turbokid/trunk; patch -p0 < trunk.diff

Then just run test and you'll see issues. Thanks!

comment:11 Changed 9 years ago by chrisz

Thank you xaka, I'll look into that.

comment:12 Changed 9 years ago by chrisz

Ok, I added your tests and was able to reproduce the problems: If you reload or invalidate a base that is used by a different template (i.e. they share the same master), then the other template will complain when rendering because one of its bases is missing.

I think the main issue here is that templates are actually loaded under two different names, depending on whether you load them directly or indirectly as master templates through the Kid importer.

The current revision r6995 takes this into account and all your tests pass. I have also simplified your tests and made them faster.

Let me know if you're satisfied with this solution, so we can close this ticket and create a new TurboKid? release.

comment:13 Changed 9 years ago by xaka

I'm very satisfied :) Thank you for your work!

comment:14 Changed 9 years ago by xaka

If you can, please let me know (here?) when the new release will be available.

comment:15 Changed 9 years ago by chrisz

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

The new TurboKid version 1.0.5 which includes this patch is already available at  PyPI.

Bugfix releases TG 1.0.10 and 1.1.1 will hopefully be released next weekend; they will require the new TurboKid version.

Thanks for your patches and feedback!

Note: See TracTickets for help on using tickets.