From f993ce26c0d95afbc13ed1b8517d37ebdc942575 Mon Sep 17 00:00:00 2001 From: Vladimir Kochnev Date: Wed, 3 Aug 2016 12:09:19 +0300 Subject: [PATCH] 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.* --- lib/axlsx/package.rb | 8 ++++++-- lib/axlsx/rels/relationship.rb | 18 +++++++++++++----- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/lib/axlsx/package.rb b/lib/axlsx/package.rb index 8a4f29dc..2581abed 100644 --- a/lib/axlsx/package.rb +++ b/lib/axlsx/package.rb @@ -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 @@ -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 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 @@ 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}. @@ -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