Skip to content

Commit

Permalink
Fix Relationship.instances cache.
Browse files Browse the repository at this point in the history
This PR aims to fix several issues with Relationship cache:

1) It's not threadsafe, so I propose to use a TLS variable for this.
2) Memory obtained by cache remains non-freed before the next run of `serialize`. I think it should be freed immediately.
3) Memory should be freed in `ensure` block to prevent memory bloating in case of exception.

*There are only two hard things in Computer Science: cache invalidation and naming things.*
  • Loading branch information
marshall-lee committed Mar 9, 2019
1 parent c593a08 commit 19b6699
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 7 deletions.
8 changes: 6 additions & 2 deletions lib/axlsx/package.rb
Original file line number Diff line number Diff line change
Expand Up @@ -100,11 +100,13 @@ def workbook=(workbook) DataTypeValidator.validate :Package_workbook, Workbook,
# File.open('example_streamed.xlsx', 'w') { |f| f.write(s.read) }
def serialize(output, confirm_valid=false)
return false unless !confirm_valid || self.validate.empty?
Relationship.clear_cached_instances
Relationship.initialize_cached_instances
Zip::OutputStream.open(output) do |zip|
write_parts(zip)
end
true
ensure
Relationship.clear_cached_instances
end


Expand All @@ -113,11 +115,13 @@ def serialize(output, confirm_valid=false)
# @return [StringIO|Boolean] False if confirm_valid and validation errors exist. rewound string IO if not.
def to_stream(confirm_valid=false)
return false unless !confirm_valid || self.validate.empty?
Relationship.clear_cached_instances
Relationship.initialize_cached_instances
zip = write_parts(Zip::OutputStream.new(StringIO.new, true))
stream = zip.close_buffer
stream.rewind
stream
ensure
Relationship.clear_cached_instances
end

# Encrypt the package into a CFB using the password provided
Expand Down
18 changes: 13 additions & 5 deletions lib/axlsx/rels/relationship.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,28 @@ class << self
# Keeps track of all instances of this class.
# @return [Array]
def instances
@instances ||= []
Thread.current[:axlsx_relationship_cached_instances] ||= []
end
# Clear cached instances.

# Initialize cached instances.
#
# This should be called before serializing a package (see {Package#serialize} and
# {Package#to_stream}) to make sure that serialization is idempotent (i.e.
# Relationship instances are generated with the same IDs everytime the package
# is serialized).
def initialize_cached_instances
Thread.current[:axlsx_relationship_cached_instances] = []
end

# Clear cached instances.
#
# This should be called after serializing a package (see {Package#serialize} and
# {Package#to_stream}) to free the memory allocated for cache.
#
# Also, calling this avoids memory leaks (cached instances lingering around
# forever).
def clear_cached_instances
@instances = []
Thread.current[:axlsx_relationship_cached_instances] = nil
end

# Generate and return a unique id (eg. `rId123`) Used for setting {#Id}.
Expand All @@ -30,7 +38,7 @@ def clear_cached_instances
# {clear_cached_instances} will automatically reset the generated ids, too.
# @return [String]
def next_free_id
"rId#{@instances.size + 1}"
"rId#{instances.size + 1}"
end
end

Expand Down
8 changes: 8 additions & 0 deletions test/rels/tc_relationship.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,14 @@ def test_instances_with_same_attributes_share_id
instance = Axlsx::Relationship.new(source_obj, Axlsx::WORKSHEET_R, 'target')
assert_equal instance.Id, Axlsx::Relationship.new(source_obj, Axlsx::WORKSHEET_R, 'target').Id
end

def test_instances_cache_is_thread_safe
cache1, cache2 = nil
t1 = Thread.new { cache1 = Axlsx::Relationship.instances }
t2 = Thread.new { cache2 = Axlsx::Relationship.instances }
[t1, t2].each(&:join)
assert_not_same(cache1, cache2)
end

def test_target_is_only_considered_for_same_attributes_check_if_target_mode_is_external
source_obj = Object.new
Expand Down
2 changes: 2 additions & 0 deletions test/tc_package.rb
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,8 @@ def test_to_stream
# this is just a roundabout guess for a package as it is build now
# in testing.
assert(stream.size > 80000)
# Cached instances should be cleared
assert(Axlsx::Relationship.instances.empty?)
end

def test_encrypt
Expand Down

0 comments on commit 19b6699

Please sign in to comment.