logstash-plugins / logstash-integration-aws

Apache License 2.0
7 stars 17 forks source link

S3 Output's Stale-sweeper is not atomic. #18

Closed yaauie closed 1 year ago

yaauie commented 1 year ago

During shutdown, The S3 Output's close method uses FileRepository#each_files to iterate over instances of FileRepository::PrefixedValue, but the stale sweeper can delete the underlying files before iteration is complete. If FileRepository#each_files were instead implemented through FileRepository#get_file, we would prevent a deleted FileRepository::PrefixedValue from being retained:

        def each_files
          @prefixed_factories.keys.each do |prefix|
            get_file(prefix) { |current| yield current }
          end
        end

Additionally, there is a separate race condition in which a file's detection-as-stale and the deletion of the underlying file descriptor is not performed in a single lock, which in theory can allow bytes to be lost if they are written in this window. This can be resolved by using a (reentrant) Monitor in place of the existing Mutex and performing both the check and change in a single operation:

        def remove_stale(k, v)
          v.with_lock do |_|
            if v.stale?
              @prefixed_factories.remove(k, v)
              v.delete!
            end
          end
        end
yaauie commented 1 year ago

Resolved by #19 and released in v7.0.1 of this plugin