Ripping blocks at 100% after commit 4cc4f5968a

[GoogleCodeExporter]

1) Please describe the steps to reproduce the situation:
a. Update to commit 48539dd7 in git
b. run rubyripper_cli.rb with a disc in
c. watch the CD be ripped properly, but pause after saying 100% and not end 
until a SIGINT is sent

2) What is the expected output? What do you see instead?
Expect the CD to eject, for the files to be moved from their temporary 
locations, and for rubyripper_cli.rb to return to the command line.

3) What version of rubyripper are you using? On what operating system? The
gtk2 of commandline interface?

4) Is this not already fixed with the latest & greatest code? See for
instructions the Source tab above.

The problem was introduced somewhere here:

http://github.com/rubyripperdev/rubyripper/compare/f8a8784...48539dd7

Most likely here: 
http://github.com/rubyripperdev/rubyripper/commit/4cc4f5968aeb74da33204a351bda
3145a8803720

I will take a look and see if I can figure out why it stops, but I don't 
know this code base very well. 

Original issue reported on code.google.com by chris.pe...@gmail.com on 8 May 2010 at 9:10

[GoogleCodeExporter]

With debug=true, it hangs right here:

Ripping progress (92.7 %)
Adding track 13 (flac) to the queue..
Ripping track 14
Expected filesize for track 14      is 35973884 bytes.
Free disk space is 213260900 MB
Minutes ripping is 4.41661756666667.
Starting to rip track 14, trial #1
cdparanoia -Z [.193232]- -d /dev/scd0 -O 0 "/home/peplin/Loudon Wainwright 
III/temp_sr0/track14_1.wav" 2>&1
Minutes ripping is 4.54340965.
Starting to rip track 14, trial #2
cdparanoia -Z [.193232]- -d /dev/scd0 -O 0 "/home/peplin/Loudon Wainwright 
III/temp_sr0/track14_2.wav" 2>&1
Analyzing files for mismatching chunks
Every chunk matched 2 times :)
MD5 sum: bc1390446e302270488f2ce9d1765199

Ripping progress (100 %)
Adding track 14 (flac) to the queue..

Original comment by chris.pe...@gmail.com on 8 May 2010 at 9:27

[GoogleCodeExporter]

Track encoding jobs are never being removed from the queue. Here's the output 
of one 
track (with some extra debugging info about the queue):

Ripping progress (12.8 %)
Encoding progress (0 %)
Adding track 1 (flac) to the queue..
Max size of queue is 3
Max threads is 3
Queue has 0 waiters
command = flac --best --delete-input-file -o "/home/peplin/Levon Helm/2009 - 
Electric 
Dirt/Levon Helm - 2009 - Electric Dirt - 01 - Tennessee Jed.flac" --tag 
ALBUM="Electric 
Dirt" --tag DATE="2009" --tag GENRE="Bluegrass" --tag DISCID="890aed0b" --tag 
ARTIST="Levon Helm" --tag TITLE="Tennessee Jed" --tag TRACKNUMBER=1 --tag 
TRACKTOTAL=11 
"/home/peplin/Levon Helm/temp_sr0/track1_1.wav" 2>&1
Ripping track 2
Expected filesize for track 2       is 35781020 bytes.
Free disk space is 212969772 MB
Minutes ripping is 0.806757366666667.
Starting to rip track 2, trial #1
cdparanoia -Z [.26903]-[.15212] -d /dev/scd0 -O 0 "/home/peplin/Levon 
Helm/temp_sr0/track2_1.wav" 2>&1
Minutes ripping is 1.01096695.
Starting to rip track 2, trial #2
cdparanoia -Z [.26903]-[.15212] -d /dev/scd0 -O 0 "/home/peplin/Levon 
Helm/temp_sr0/track2_2.wav" 2>&1
Analyzing files for mismatching chunks
Every chunk matched 2 times :)
MD5 sum: 0cae44d192ab45238d14076ffbfe8796

I expect to see "Removing track ... from the queue" but it never returns to the 
addTrack function to print that, or more importantly remove the job from the 
queue.

Original comment by chris.pe...@gmail.com on 8 May 2010 at 11:31

[GoogleCodeExporter]

This had to do with a conflict between my flacsettings and the addition of 
rubyripper 
trying to remove the temp file. 

My settings had:
flacsettings=--best --delete-input-file

and rr_lib.rb now has:
File.delete(@out.getTempFile(track, 1)) if (@tasks[track] -= 1) == 0

The File.delete would throw an exception because the file was already deleted 
by FLAC, 
causing the synchronized Monitor block to exit and the thread to die before it 
dequeues 
its job. 

Perhaps consider wrapping the File.delete with a begin/rescue. I've done that 
in my 
rubyripper fork: 
http://github.com/peplin/rubyripper/commit/06f6032f94e04d66be6513f57d2da6ca7e00a
396

Original comment by chris.pe...@gmail.com on 9 May 2010 at 12:40

[GoogleCodeExporter]

I don't see the benefit of --delete-input-file switch here. I recommend to 
simply 
remove it. The temporary wav files will be deleted automatically. Just like any 
other 
ripper program would do.

Original comment by boukewou...@gmail.com on 16 May 2010 at 4:23

• Changed state: WontFix

[GoogleCodeExporter]

In previous versions of rubyripper, I had to use --delete-input-file switch 
with the 
FLAC encoder, or else the temporary .wav files would not be deleted. If I leave 
that 
in my settings file with HEAD, encoding essentially breaks.

I'm not saying there's any benefit to keeping the flag, and I'm happy to let 
rubyripper be the one to do the deleting. Becoming non-functional if the 
encoder 
behaves slightly differently than expected (ie. deleting the input file) isn't 
okay, 
though.  I can think of other situations where the File.delete call may throw 
an 
exception, which would trigger this bug as well.

Original comment by chris.pe...@gmail.com on 16 May 2010 at 7:33

[GoogleCodeExporter]

If you can show me any other ripper that isn't crashing with the switch, I'll 
think 
about it. I will accept any issues in case the wav files are not deleted though.

Original comment by boukewou...@gmail.com on 16 May 2010 at 7:49

[GoogleCodeExporter]

Can you clarify what you mean? I'm not sure I understand.

If you don't want to add the exception block, there should be mention in the 
documentation that there are certain flags you cannot pass to your encoder.

Original comment by chris.pe...@gmail.com on 16 May 2010 at 7:55

[GoogleCodeExporter]

It's not normal behaviour for an encoder to remove the inputfile. As far as I 
know, 
there is no single other codec around that does the same thing. In rubyripper 
there 
is already logic in place to ensure the outputted wav is there completely.

If any other process is removing the wav file, I find this to be an user error, 
not a 
program error. I guess other rippers will have problems with the switch as 
well. 
Let's just assume I find this issue really worthwhile. The next issue will be: 
why is 
my ogg encoding failing? Some research will show that the flac is executed 
first and 
is removing the input file. This is an endless road. Besides, I don't like to 
"rescue" code. It hides bad programming practice.

If anything should be done about the switch, then it is to automatically filter 
it 
out.

Original comment by boukewou...@gmail.com on 16 May 2010 at 9:19

[GoogleCodeExporter]

The multiple encoder case is a good one, but rubyripper will still just hang 
with the 
current implementation - someone will not just be asking why is their ogg 
encoding 
failing, but why is every encode after max_threads failing, and why is there a 
strange temp directory?

If you really don't catch the exception, you could check for the existence of 
the 
file before trying to delete it. If you don't think this is important (and 
that's 
obviously your call) I just wanted to let you know that it is possible for the 
encoding threads to exit prematurely without releasing their lock on the thread 
pool 
queue. Thanks for talking it out with me.

Original comment by chris.pe...@gmail.com on 16 May 2010 at 10:00

[GoogleCodeExporter]

I keep this one open for the sake of the discussion ;)

Original comment by boukewou...@gmail.com on 17 May 2010 at 6:14

• Changed state: New

[GoogleCodeExporter]

Perhaps I have been a little aggressive in my reactions above, sorry for that. 
On the 
other hand, your reasoning style is quite centered around your own situation. 
Not 
many people will use flags like --delete-input-file. Also, you are making 
claims 
without supporting them with arguments, proof or further explanation.

Your claim that every encode after max_threads is failing is simply false. I've 
tried 
with setting the max threads to 1 and having 4 encodings selected. No problems 
at 
all, every encoding has finished just like I expected.

To finish this discussion with a simple solution I've just commited:
http://github.com/rubyripperdev/rubyripper/commit/d8b97016d74e895d63cb7670d1d2af
28d81
75053

This commit will prevent the crash in your particular situation.

Original comment by boukewou...@gmail.com on 17 May 2010 at 9:09

• Changed state: Fixed

[GoogleCodeExporter]

Thanks for committing a fix.

For posterity, my claim was that the encodings fail if the input files are 
deleted by 
the encoder. I assumed that the log output included and debugging I did to 
start this 
discussion was enough evidence.

Original comment by chris.pe...@gmail.com on 18 May 2010 at 12:25