tjko / jpegoptim

jpegoptim - utility to optimize/compress JPEG files
http://www.iki.fi/tjko/projects.html
GNU General Public License v3.0
1.56k stars 116 forks source link

Fix parallel processing and reading file list looping infinitely #148

Closed Cubittus closed 1 year ago

Cubittus commented 1 year ago

This change fflushes files_from before forking.

Forking a process with open file streams requires that the stream be fflushed before the fork (even read only streams). Otherwise when the child exit()s it will clobber the seek position of the stream in the parent process, often resulting in an infinite loop as the end of the file is never found.

Technical details at: https://stackoverflow.com/questions/50110992/why-does-forking-my-process-cause-the-file-to-be-read-infinitely/50112169#50112169

tjko commented 1 year ago

This would seem to only affect Linux and not be issue on MacOS (or on BSD variants)?

Wouldn't it be safer to simply close this stream at the beginning of the child process (or call _exit() to terminate child)?

Cubittus commented 1 year ago

I think all three options are equally effective/safe:

I just picked fflush, as it seemed simplest.

I don't know MacOS or BSD too well, and this may only be a linux/glibc thing. The SO link above includes links to POSIX docs describing the behaviour as part of the standard.

I've been using jpegoptim with the patch locally for a couple of days, processing a few thousand files in batches of a few hundred using -w 16 and --files-from=..., and encountered no problems.

tjko commented 1 year ago

@Cubittus, I assume you were having issues thus this pull request? Would you be able to quickly test if using fclose() in the beginning of the child process works for you as well?
Since it would seem to be more "portable" solution (less likely to have any side effects on some other OS, etc.), compared to using fflush() on a input stream.

Cubittus commented 1 year ago

Yes, I was seeing an inifinite loop. The program would process around 100 files, then give a file-not-found error on a bogus filename not found in the input file list (that was composed of bits of two different filenames), then start from the beginning of the file list again.

A bit debugging found that ftell on the files_from handle became invalid after the first few files were processed - likely after the first sub-process exited, so I read up on how file handles behave during a fork.

A quick google of fopen fork led me to the above article explaining the behaviour.

I just tested the above new patch (fclose in child) on a list of 500 files with -w 16, and it fixes the problem too. All testing was done with the -t option and checking that the total saved bytes was equal.

I'm not aware of any compatibility issues with fflush on input files, but am not too familiar with MacOS or *BSDs.

tjko commented 1 year ago

Thanks for the fix. Since it seems like fflush() behavior for input streams was not defined until in POSIX.1-2008, I was thinking it should be safer to use fclose() instead, in case someone want to use jpegoptim on some old OS, etc...