Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix parallel processing and reading file list looping infinitely #148

Merged
merged 2 commits into from
May 2, 2023

Conversation

Cubittus
Copy link
Contributor

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

This change `fflush`es `files_from` before `fork`ing.

Forking a process with open file streams requires that the stream be
`fflush`ed 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
Copy link
Owner

tjko commented May 1, 2023

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
Copy link
Contributor Author

Cubittus commented May 1, 2023

I think all three options are equally effective/safe:

  • fflush before fork
  • fclose as child starts
  • _exit instead of exit

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
Copy link
Owner

tjko commented May 2, 2023

@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
Copy link
Contributor Author

Cubittus commented May 2, 2023

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 tjko merged commit 30966d6 into tjko:master May 2, 2023
@tjko
Copy link
Owner

tjko commented May 2, 2023

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants