Fix process-global memory leak in folded_declaration_cache#180
Conversation
In long-lived processes (e.g., Sidekiq workers rendering emails via
Premailer), the process-global `@folded_declaration_cache` on
`CssParser::Parser` accumulates entries from every CSS parse operation
and is never cleared, causing unbounded memory growth.
The class-level `@folded_declaration_cache` (and its `attr_reader`) on
`CssParser::Parser` was defined as a class instance variable that
persists for the lifetime of the process. While `reset!` (called in
`initialize`) creates a per-instance `@folded_declaration_cache`, the
class-level variable and accessor remained, leaking the global
reference.
Similarly, `CssParser.merge` set a module-level
`@folded_declaration_cache = {}` on every call, which also persisted
at the module level.
This commit:
- Removes the class-level `@folded_declaration_cache` and its
`class << self; attr_reader` from `CssParser::Parser`
- Removes the module-level `@folded_declaration_cache` assignment
in `CssParser.merge`
- The per-instance cache (set in `reset!`) is already properly scoped
and gets garbage collected with the `Parser` instance
- Adds tests verifying the cache is per-instance and not class-level
Amp-Thread-ID: https://ampcode.com/threads/T-019c8b1f-829d-70cd-83c5-3409a635e616
Co-authored-by: Amp <amp@ampcode.com>
| # Class variable? see http://www.oreillynet.com/ruby/blog/2007/01/nubygems_dont_use_class_variab_1.html | ||
| #++ | ||
| @folded_declaration_cache = {} | ||
| class << self; attr_reader :folded_declaration_cache; end |
There was a problem hiding this comment.
The class-level @folded_declaration_cache and its accessor were dead code. Nothing ever reads Parser.folded_declaration_cache (the class method). All actual usage (lines 693, 698, 702) references @folded_declaration_cache on self — the instance. The reset! method (called from initialize) creates a fresh per-instance @folded_declaration_cache = {} that shadows the class-level one. So the class-level hash just accumulated stale data forever with nothing reading it. Same story for the @folded_declaration_cache = {} reset in CssParser.merge — it was setting it on the CssParser module object, which nothing ever reads.
| # TODO: declaration_hashes should be able to contain a RuleSet | ||
| # this should be a Class method | ||
| def self.merge(*rule_sets) | ||
| @folded_declaration_cache = {} |
There was a problem hiding this comment.
This line was setting @folded_declaration_cache on the CssParser module object itself — completely separate from both the class-level one on Parser and the per-instance one created in reset!. Three different Ruby objects all had an ivar with the same name, but nothing ever read from this one.
|
@grosser I think this one is ready too! Somewhat related to but independent from premailer/premailer#472. |
grosser
left a comment
grosser
left a comment
There was a problem hiding this comment.
the tests seem a bit unnecessary, if the code is no longer there then they can go too ?
The tests were asserting the absence of removed code — unnecessary. Amp-Thread-ID: https://ampcode.com/threads/T-019c8b1f-829d-70cd-83c5-3409a635e616 Co-authored-by: Amp <amp@ampcode.com>
@grosser updated! |
|
I'm working on fixing CI, then merging this |
|
(Unrelated) test fix: #181 |
|
2.0.0 |
2 participants
Problem
In long-lived processes (e.g., Sidekiq workers rendering emails via Premailer), the process-global
@folded_declaration_cacheonCssParser::Parseraccumulates entries from every CSS parse operation and is never cleared, causing unbounded memory growth.Root cause
CssParser::Parserdefined a class-level instance variable:This hash persists for the lifetime of the Ruby process. While
reset!(called ininitialize) creates a per-instance@folded_declaration_cache, the class-level variable and its public accessor remained, leaking a global reference that never gets GC'd.Similarly,
CssParser.mergeset@folded_declaration_cache = {}at the module level on every call.Fix
@folded_declaration_cacheand itsattr_readerfromCssParser::Parser@folded_declaration_cacheassignment inCssParser.mergereset!) is already properly scoped and gets garbage collected with theParserinstanceTests
Added tests verifying:
folded_declaration_cacheaccessor exists