each_buffer causes ReaderClosedError

[kylekyle]

each_buffer closes the reader which causes some unintuitive errors:

reader = WaveFile::Reader.new(ARGV.first)

reader.each_buffer do |buffer|
  puts "Reading buffer ..."
end

reader.close # throws errors since reader is already closed

This also means you can never use each_buffer in a Reader block:

WaveFile::Reader.new(ARGV.first) do |reader|
  reader.each_buffer do |buffer|
    puts "Reading buffer ..."
  end 
end # error thrown when block completes

I personally don't think each_buffer should close the reader. What if I want to make a second pass?

If it is important that each_block closes the Reader, then it should be renamed each_block! since it has destructive side-effects. If each_block is going to continue to close the Reader, then you also won't wantReader#close to throw an error when called on a closed Reader or you'll never be able to use each_block in a Reader block.

[jstrait]

Hey @kylekyle, thanks for the feedback!

I think the original reason I made each_buffer close the file was so that you could read an entire file by doing this:

Reader.new("my_file.wav").each_buffer do |buffer|
  # Do something with buffer
end

If it didn't automatically close the file, you would have to do this instead:

Reader.new("my_file.wav") do |reader|
  reader.each_buffer do |buffer|
    # Do something with buffer
  end
end

Or this:

reader = Reader.new("my_file.wav")
reader.each_buffer do |buffer|
  # Do something with buffer
end
reader.close

(Both examples currently don't work, as you pointed out).

It's possible that optimizing for succinctness here is the wrong trade-off, because it reduces the flexibility to use each_buffer in different ways, and it could also be confusing. To be honest it's not something I've thought about for awhile, but as I look at it now the current interface does look a little weird to me.

I see a couple of options:

1. Change Reader.close to no longer raise an error if the Reader is already closed, as you pointed out.
2. Change each_buffer to no longer close the file when the block exits, as you pointed out.
3. Add a new way of opening a file for buffered reading. E.g. something like:

   WaveFile::Reader.read_buffered("my_file.wav", <optional buffer size>) do |buffer|
     # Do something with buffer
   end

1.) Seems like the most appealing option. I originally made Reader.close raise an error if the file is already closed to match the behavior of IO.close. However, it looks like IO.close no longer raises an error as of Ruby 2.3, and I'm not sure of another reason why this should raise an error in that scenario. It seems like changing this behavior would allow the examples you gave to work.

I'm wary of changing each_buffer to no longer close the file because it would cause unexpected behavior in existing code that uses the Reader.new.each_block {} pattern - if you updated the gem without changing you code, your code would silently change to leave the file open forever. I'd have to think more about the trade-off between a potentially better interface vs. breaking existing code.

I'd have to think more about 3.)

Finally, is there a specific use case that you aren't able to do based on the current behavior? I.e., one work-around would be to use the Reader.new.each_buffer {} pattern, unless what you are trying to do isn't possible with that.

[jstrait]

Hey @kylekyle, I released v1.0.0 of the gem a few days ago, and as of that release Reader.close and Writer.close no longer raise an error if the Reader/Writer is already closed. I think that should allow the code in your examples to work.

I’ll keep in mind the other suggestions you made as possible changes for the future, but I’m not planning to make any immediate changes.


Thanks for raising this issue!

[kylekyle]

Excellent! Thanks!