diff options
| author | Vladimir Kochnev <[email protected]> | 2016-08-03 12:09:19 +0300 |
|---|---|---|
| committer | Vladimir Kochnev <[email protected]> | 2019-10-02 18:06:35 +0300 |
| commit | 913003eaf0456e4645cad91f2622354deae10841 (patch) | |
| tree | 27293f11b37df246eaf15da6a062bbfade8882bb | |
| parent | c73b36d80f6a07862ead30a593f62b4797926110 (diff) | |
| download | caxlsx-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.rb | 8 | ||||
| -rw-r--r-- | lib/axlsx/rels/relationship.rb | 18 | ||||
| -rw-r--r-- | test/rels/tc_relationship.rb | 8 | ||||
| -rw-r--r-- | test/tc_package.rb | 2 |
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 |
