Skip to content

Close temporary file descriptors and remove the unused warp matrix - #86

Open
Arthur031221 wants to merge 1 commit into
poldracklab:masterfrom
Arthur031221:fix/close-tmpfile-fds
Open

Arthur031221 wants to merge 1 commit into
poldracklab:masterfrom
Arthur031221:fix/close-tmpfile-fds

Conversation

@Arthur031221

Copy link
Copy Markdown

Anyone who defaces many images from one Python process (a loop over a BIDS dataset calling deface_image) leaks four open file descriptors per image, which eventually fails with OSError: Too many open files; separately, every run leaves one temporary .mat file behind even with cleanup on.

generate_tmpfiles discards the first value of tempfile.mkstemp, which is an open descriptor, so the four descriptors are never closed. deface_image also never removes warped_mask_mat: it is not part of the return value, so neither forcecleanup=True nor the CLI's cleanup_files(...) call can reach it.

Reproduction on current master (Linux; FSL is not installed here, so FLIRT.run is stubbed to write a blank image and the second check only shows that the file is not removed; TMPDIR pointed at an empty directory):

fds leaked after 10 generate_tmpfiles calls: 40
temp files left after forcecleanup=True: ['tmpq5146b62.mat']

With this change both numbers are zero.

Changes:

  • generate_tmpfiles closes each descriptor right after mkstemp; the returned paths and their order are unchanged.
  • deface_image removes warped_mask_mat once the second FLIRT call has finished.

Two tests added in tests/test_utils.py. Both fail on master (descriptor count grows; one .mat file remains) and pass here. Local run on Linux, Python 3.12: 7 passed, 1 skipped (the skip is test_deface_image, which needs FSL, so nothing here ran against a real FSL install). ruff check and ruff format --check are clean at v0.15.12, the version pinned in .pre-commit-config.yaml.

Targets master, where #83 and #85 were merged.

@effigies effigies left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about instead doing:

with tempfile.TemporaryDirectory(prefix='pydeface', delete=forcecleanup) as tmpdir:
    if verbose:
        print(f'Temporary directory: {tmpdir}')
    template_reg_mat = os.path.join(tmpdir, 'reg.mat')
    ...

There's no need to close file descriptors, because they aren't opened. The generate_tmpfiles helper goes away, and the files are cleaned up based on the forcecleanup flag.

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