Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cached blocks should be stored per request #101

Closed
penx opened this issue Jun 14, 2016 · 2 comments · Fixed by #149
Closed

Cached blocks should be stored per request #101

penx opened this issue Jun 14, 2016 · 2 comments · Fixed by #149

Comments

@penx
Copy link

penx commented Jun 14, 2016

The cached blocks object should be specific to each request, rather than a singleton shared by all requests. After a request has completed, the object can then be removed from memory, ensuring no memory leaks are created. In addition it would prevent conflicts between simultaneous page loads that are using async helpers.

This would resolve issues #99 and #100
@jtwebman
Copy link
Collaborator

@penx This might be hard with the current design of the API. Express doesn't give us the req or res object when render is called. Express render was not really designed with async in mind as it expects all user data to come into the render. I wonder if this library should have never excited with async and forced the programmer to first fetch all data first before calling render but with that said this is what I found looking at this issue.

Maybe a better way to solve the memory leak is to use a smarter cache with TTLs.

As for the async wrong content are you sure? The library does generate a new ID for each async content request. Let me see if I can create a test that causes this issue.

jtwebman added a commit to jtwebman/express-hbs that referenced this issue Aug 27, 2018
@jtwebman
Fix async render and block cache.
5e28c79 
Fixed TryGhost#144 registerAsyncHelper using the wrong replace call
Fixes TryGhost#143 Update handlebars to 4.0.8 (did latest handlebars)
Fixed TryGhost#101 Cached blocks should be stored per request
Fixed TryGhost#100 Conflict between content blocks
Fixed TryGhost#99 Blocks prone to memory leaks

After digging round most of the weekend I fixed the block and sync cache issues by storing them
on the options which is setup once per request and passed to everything that needed it. This
also fixes the issue with using replace and special strings that can do funny things with
JavaScript replace vs using the function version.

I also update TravisCI to run the newer node versions for tests and update all the dependencies
to the latest versions.
@jtwebman jtwebman mentioned this issue Aug 27, 2018
Merged
@jtwebman
Copy link
Collaborator

@penx Ok I think I figured it out after debugging for a few hours. See PR #149 it has this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix async render and block cache. jtwebman/express-hbs
2 participants
@jtwebman @penx