Skip to content

Commit 4346ab0

Browse files
sokolikpampagent
andcommitted
Fix process-global memory leak in folded_declaration_cache
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>
1 parent 2ef7dca commit 4346ab0

3 files changed

Lines changed: 42 additions & 8 deletions

File tree

lib/css_parser.rb

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,6 @@ module CssParser
5252
# TODO: declaration_hashes should be able to contain a RuleSet
5353
# this should be a Class method
5454
def self.merge(*rule_sets)
55-
@folded_declaration_cache = {}
56-
5755
# in case called like CssParser.merge([rule_set, rule_set])
5856
rule_sets.flatten! if rule_sets[0].is_a?(Array)
5957

lib/css_parser/parser.rb

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -31,12 +31,6 @@ class Parser
3131
# Array of CSS files that have been loaded.
3232
attr_reader :loaded_uris
3333

34-
#--
35-
# Class variable? see http://www.oreillynet.com/ruby/blog/2007/01/nubygems_dont_use_class_variab_1.html
36-
#++
37-
@folded_declaration_cache = {}
38-
class << self; attr_reader :folded_declaration_cache; end
39-
4034
def initialize(options = {})
4135
@options = {
4236
absolute_paths: false,
Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
# frozen_string_literal: true
2+
3+
require_relative 'test_helper'
4+
5+
class CssParserFoldedDeclarationCacheTest < Minitest::Test
6+
include CssParser
7+
8+
def test_folded_declaration_cache_is_per_instance
9+
cp1 = Parser.new
10+
cp2 = Parser.new
11+
12+
cp1.add_block!('p { color: red; }')
13+
cp2.add_block!('p { color: blue; }')
14+
15+
cache1 = cp1.send(:instance_variable_get, :@folded_declaration_cache)
16+
cache2 = cp2.send(:instance_variable_get, :@folded_declaration_cache)
17+
18+
refute_same cache1, cache2
19+
end
20+
21+
def test_folded_declaration_cache_does_not_persist_across_instances
22+
cp1 = Parser.new
23+
cp1.add_block!('p { color: red; }')
24+
25+
cp2 = Parser.new
26+
cache = cp2.send(:instance_variable_get, :@folded_declaration_cache)
27+
28+
assert_empty cache
29+
end
30+
31+
def test_no_class_level_folded_declaration_cache
32+
refute Parser.respond_to?(:folded_declaration_cache),
33+
'Parser should not have a class-level folded_declaration_cache accessor'
34+
end
35+
36+
def test_cache_is_reset_on_new_instance
37+
cp = Parser.new
38+
cache = cp.send(:instance_variable_get, :@folded_declaration_cache)
39+
assert_instance_of Hash, cache
40+
assert_empty cache
41+
end
42+
end

0 commit comments

Comments
 (0)