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 Aug 3, 2016
1 parent d136835 commit f993ce2
Show file tree
Hide file tree
Showing 2 changed files with 19 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

0 comments on commit f993ce2

Please sign in to comment.