Support for relative paths introduces cache performance bug #378
Comments
|
I'm taking a look at the issue, I'll add some unit tests and propose a fix. |
|
I'm working on unit tests now, if I can get my install --dev to complete... On Sat, Feb 21, 2015 at 11:01 AM, Samy Pessé notifications@github.com
*THOMAS BOUTELL, *DEV & OPS |
|
Hmm, my test passed. Is the cache always active? On Sat, Feb 21, 2015 at 11:04 AM, Tom Boutell tom@punkave.com wrote:
*THOMAS BOUTELL, *DEV & OPS |
|
I added a test but it's passing: https://github.com/SamyPesse/nunjucks/commit/af62c97713e6d81628a4c3db581f041116d26ea4 Let me know if the test is correctly covering the issue. |
|
Same for me ^^ I'm taking a look at why. |
|
Very interesting. I'm going to try logging a bit. On Sat, Feb 21, 2015 at 11:08 AM, Samy Pessé notifications@github.com
*THOMAS BOUTELL, *DEV & OPS |
|
(We wrote almost identical tests.) On Sat, Feb 21, 2015 at 11:08 AM, Tom Boutell tom@punkave.com wrote:
*THOMAS BOUTELL, *DEV & OPS |
|
It looks like the cache key is the correct absolue path (like it should be):
|
|
You're right, it is, however the test for a cache match is done before the So this is actually a cache performance bug, not a wrong-result bug. I will retitle the bug. On Sat, Feb 21, 2015 at 11:11 AM, Samy Pessé notifications@github.com
*THOMAS BOUTELL, *DEV & OPS |
boutell
changed the title
|
I have updated the issue's description to cover the cache performance issue correctly and suggested a solution that involves invoking resolve for each loader and stopping if one of the loaders claims responsibility. A loader should not do so unless the template actually exists. I think this requires making resolve async-capable (but not async-mandatory, since synchronous template support is still important to many, including my coworkers and I (: ) |
|
https://github.com/SamyPesse/nunjucks/commit/0f4a85c8b8aad5a48ea1491eeff49594dd43f6f4 Does it sound good to you? |
|
Hmm. That would certainly work for us. I think it would be a little safer in the On Sat, Feb 21, 2015 at 11:12 AM, Tom Boutell tom@punkave.com wrote:
*THOMAS BOUTELL, *DEV & OPS |
|
Just to clarify, my concern is that right now, loaders are a straightforward filter pattern: the first loader to claim a template by actually returning a happy result wins. There's no ambiguity. When we call "resolve" on all the loaders prior to that point, we introduce an ambiguity. Any loader might think it knows how to resolve that. They may have different ideas of how to resolve it. And since it's synchronous, they can't really check to be sure it's really going to work when you ask them for the source code (at least not in the general case). So you could get a resolve() result from loader 1 when in reality only loader 2 was capable of loading the template. |
|
One solution could be to prepend an For example, the cache key become: |
|
Oh right, we can do that, can't we. It's the loader that issues "update" On Sat, Feb 21, 2015 at 11:51 AM, Samy Pessé notifications@github.com
*THOMAS BOUTELL, *DEV & OPS |
|
Here is what I believe is a better solution: https://github.com/SamyPesse/nunjucks/commit/17bc3f0b55241636a33faba7767f45a94fa02275 I just moved the |
|
Perfect! I was going to suggest moving responsibility for caching into the On Sat, Feb 21, 2015 at 12:13 PM, Samy Pessé notifications@github.com
*THOMAS BOUTELL, *DEV & OPS |
|
(Really appreciate all your work on this!) On Sat, Feb 21, 2015 at 12:35 PM, Tom Boutell tom@punkave.com wrote:
*THOMAS BOUTELL, *DEV & OPS |
See #349 cc: @SamyPesse
With the new support for relative paths in folder names, the same "name" can refer to two completely different templates.
The cache storage code does in fact address this, storing the cached template under its resolved name.
However the cache fetch code runs before the resolve call.
The correct resolution would be to put the resolve call before the cache fetch code. However there is a wrinkle: nunjucks permits multiple loaders at once. And the "resolve" call currently is not responsible for saying "this is the correct resolution and I'm sure this file actually exists and I'm taking responsibility for it, so stop looking for other loaders."
I think the "resolve" API needs to be tried for each configured loader, and must become responsible for not returning a happy result unless the file at the resolved path actually exists.
The text was updated successfully, but these errors were encountered: