jstrait / wavefile

A Ruby gem for reading and writing sound files in Wave format (*.wav)
https://wavefilegem.com
MIT License
208 stars 24 forks source link

No Header Is Written When Exception Occurs While Writing Inside a Block #5

Closed jstrait closed 11 years ago

jstrait commented 11 years ago

For example:

Writer.new("error.wav", format) do |writer|
  writer.write(buffer)
  x = 1 / 0
end

When this is run, the error.wav will exist on disk, but will not be playable because the Writer was never closed.

Adding an ensure clause to the block_given? section of Writer.new should resolve this.

jamestunnell commented 11 years ago

I'll work on this issue, but I don't think I have permissions to assign it to myself.

jamestunnell commented 11 years ago

I committed changes to address this issue on my fork: https://github.com/jamestunnell/wavefile

You can pull in the changes if you think they take care of the issue (tests are there as well).

jamestunnell commented 11 years ago

The exact commit is https://github.com/jamestunnell/wavefile/commit/8e75345814

jstrait commented 11 years ago

Thanks for the quick response!

Took a look at the commit. Thanks for the well documented and easy to read tests. Only comment is around the test_exception_without_block test. When not using a block, it's the client's responsibility to make sure the Writer gets closed, which can be done using begin/ensure for example. The situation in this test of the header not being written could be resolved by closing the file in an ensure block. So it sort of seems like it's testing that if the client does the wrong thing, they get the wrong result.

What it ultimately seems to be testing is that if the file hasn't been closed yet, you won't be able to read data out of it. It could possibly be simplified by removing the begin/rescue and ZeroDivisionError, and just reading the file before the Writer is closed:

writer = Writer.new("#{OUTPUT_FOLDER}/exception_without_block.wav", format)
writer.write(Buffer.new([1, 2, 3, 4], format))

reader = Reader.new("#{OUTPUT_FOLDER}/exception_without_block.wav")
assert_equal(0, reader.samples_remaining)
jamestunnell commented 11 years ago

Yeah, I guess that test is a bit pointless. Not hard to take it out though!

jstrait commented 11 years ago

Awesome, thanks! I've merged in your commits, so closing the issue.