Skip to content

Commit

Permalink
Print into Rails log when streaming fails
Browse files Browse the repository at this point in the history
It can happen that the serving gets aborted, and in a lot of cases streaming Rack bodies will not relay exceptions into the APM system like Appsignal etc. It can be useful to at least print the message into the log then.

Closes #74
  • Loading branch information
julik committed Feb 12, 2024
1 parent 8cef8c2 commit c5a2e28
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 0 deletions.
22 changes: 22 additions & 0 deletions lib/zipline/zip_generator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,23 @@ def initialize(files, **kwargs_for_streamer)
def each(&block)
return to_enum(:each) unless block_given?
@body.each(&block)
rescue => e
# Since most APM packages do not trace errors occurring within streaming
# Rack bodies, it can be helpful to print the error to the Rails log at least
error_message = "zipline: an exception (#{e.inspect}) was raised when serving the ZIP body."
error_message += " The error occurred when handling #{@filename.inspect}" if @filename
logger.error(error_message)
raise
end

def handle_file(streamer, file, name, options)
file = normalize(file)

# Store the filename so that a sensible error message can be displayed in the log
# if writing this particular file fails
@filename = name
write_file(streamer, file, name, options)
@filename = nil
end

# This extracts either a url or a local file from the provided file.
Expand Down Expand Up @@ -86,6 +98,16 @@ def is_io?(io_ish)

private

def logger
# Rails is not defined in our tests, and might as well not be defined
# elsewhere - or the logger might not be configured correctly
if defined?(Rails.logger) && Rails.logger
Rails.logger
else
Logger.new($stderr)
end
end

def is_active_storage_attachment?(file)
defined?(ActiveStorage::Attachment) && file.is_a?(ActiveStorage::Attachment)
end
Expand Down
34 changes: 34 additions & 0 deletions spec/lib/zipline/zipline_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,20 @@ def download_zip
]
zipline(files, 'myfiles.zip', auto_rename_duplicate_filenames: false)
end

class FailingIO < StringIO
def read(*)
raise "Something wonky"
end
end

def download_zip_with_error_during_streaming
files = [
[StringIO.new("File content goes here"), "one.txt"],
[FailingIO.new("This will fail half-way"), "two.txt"]
]
zipline(files, 'myfiles.zip', auto_rename_duplicate_filenames: false)
end
end

it 'passes keyword parameters to ZipTricks::Streamer' do
Expand All @@ -31,4 +45,24 @@ def download_zip

expect(headers['Content-Disposition']).to eq("attachment; filename=\"myfiles.zip\"; filename*=UTF-8''myfiles.zip")
end

it 'sends the exception raised in the streaming body to the Rails logger' do
fake_rack_env = {
"HTTP_VERSION" => "HTTP/1.0",
"REQUEST_METHOD" => "GET",
"SCRIPT_NAME" => "",
"PATH_INFO" => "/download",
"QUERY_STRING" => "",
"SERVER_NAME" => "host.example",
"rack.input" => StringIO.new,
}
expect(ZipTricks::Streamer).to receive(:new).with(anything, auto_rename_duplicate_filenames: false).and_call_original
fake_logger = double()
expect(Logger).to receive(:new).and_return(fake_logger)
expect(fake_logger).to receive(:error).with(instance_of(String))

expect {
FakeController.action(:download_zip_with_error_during_streaming).call(fake_rack_env)
}.to raise_error(/Something wonky/)
end
end

0 comments on commit c5a2e28

Please sign in to comment.