summaryrefslogtreecommitdiffhomepage
diff options
context:
space:
mode:
authorVladimir Kochnev <[email protected]>2016-08-03 12:09:19 +0300
committerVladimir Kochnev <[email protected]>2019-10-02 18:06:35 +0300
commit913003eaf0456e4645cad91f2622354deae10841 (patch)
tree27293f11b37df246eaf15da6a062bbfade8882bb
parentc73b36d80f6a07862ead30a593f62b4797926110 (diff)
downloadcaxlsx-913003eaf0456e4645cad91f2622354deae10841.tar.gz
caxlsx-913003eaf0456e4645cad91f2622354deae10841.zip
Fix Relationship.instances cache.
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.*
-rw-r--r--lib/axlsx/package.rb8
-rw-r--r--lib/axlsx/rels/relationship.rb18
-rw-r--r--test/rels/tc_relationship.rb8
-rw-r--r--test/tc_package.rb2
4 files changed, 29 insertions, 7 deletions
diff --git a/lib/axlsx/package.rb b/lib/axlsx/package.rb
index da661b34..2e9e14fc 100644
--- a/lib/axlsx/package.rb
+++ b/lib/axlsx/package.rb
@@ -100,11 +100,13 @@ module Axlsx
# 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
@@ -113,11 +115,13 @@ module Axlsx
# @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
diff --git a/lib/axlsx/rels/relationship.rb b/lib/axlsx/rels/relationship.rb
index 99dad367..66f53b2a 100644
--- a/lib/axlsx/rels/relationship.rb
+++ b/lib/axlsx/rels/relationship.rb
@@ -8,20 +8,28 @@ module Axlsx
# 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}.
@@ -30,7 +38,7 @@ module Axlsx
# {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
diff --git a/test/rels/tc_relationship.rb b/test/rels/tc_relationship.rb
index add1654f..845f789e 100644
--- a/test/rels/tc_relationship.rb
+++ b/test/rels/tc_relationship.rb
@@ -13,6 +13,14 @@ class TestRelationships < Test::Unit::TestCase
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
diff --git a/test/tc_package.rb b/test/tc_package.rb
index d89f35b0..4bd0d429 100644
--- a/test/tc_package.rb
+++ b/test/tc_package.rb
@@ -236,6 +236,8 @@ class TestPackage < Test::Unit::TestCase
# 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