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

Close tempfile handles. #5506

Merged
merged 3 commits into from Feb 15, 2018

Conversation

Projects
None yet
5 participants
@davebx
Contributor

davebx commented Feb 12, 2018

The old code would create four temporary files whenever adding or removing repository files, and these file handles would remain open after the files were deleted in the subsequent cleanup methods, leading to TTS reporting too many open files after a certain time running.

@davebx davebx added this to the 18.05 milestone Feb 12, 2018

@nsoranzo

This comment has been minimized.

Member

nsoranzo commented Feb 12, 2018

Maybe a cleaner and extensible solution may be to create a temporary config directory with tempfile.mkdtemp(), then the various config files would use their standard name inside the temp dir, and the __exit__() method could just remove the temp dir with shutil.rmtree().

@natefoo

This comment has been minimized.

Member

natefoo commented Feb 12, 2018

+1 to the suggestion by @nsoranzo. Also, when creating tempfiles (or tempdirs) it's useful to prefix them with something identifying the part of the code creating them so you know what they are if you find them lying around.

@davebx

This comment has been minimized.

Contributor

davebx commented Feb 12, 2018

@nsoranzo agreed and implemented.

os.remove(path)
except Exception:
pass
try:

This comment has been minimized.

@nsoranzo

nsoranzo Feb 12, 2018

Member

Not sure the try/except is needed now, the tmp dir should have always been created, so shutil.rmtree(self.temporary_path) shouldn't fail.

This comment has been minimized.

@davebx

davebx Feb 12, 2018

Contributor

I suppose the possibility is minuscule that some other code would have removed it before reaching that point.

This comment has been minimized.

@natefoo

natefoo Feb 13, 2018

Member

Agreed this could happen so it's best to catch it, but could you turn that into:

try:
    shutil.rmtree(self.temporary_path)
except (IOError, OSError):
    log.exception('Temporary directory removal failed')
@nsoranzo

This comment has been minimized.

Member

nsoranzo commented Feb 12, 2018

Thanks @davebx, fantastic!

@davebx

This comment has been minimized.

Contributor

davebx commented Feb 12, 2018

@natefoo this undoubtedly affects MTS as well, should I maybe retarget it for 18.01 and merge back when accepted?

os.remove(path)
except Exception:
pass
try:

This comment has been minimized.

@natefoo

natefoo Feb 13, 2018

Member

Agreed this could happen so it's best to catch it, but could you turn that into:

try:
    shutil.rmtree(self.temporary_path)
except (IOError, OSError):
    log.exception('Temporary directory removal failed')
@natefoo

This comment has been minimized.

Member

natefoo commented Feb 13, 2018

A backport would be nice. I'd say we can call this a bug due to the number of open files.

@dannon dannon merged commit 41a3196 into galaxyproject:dev Feb 15, 2018

4 of 6 checks passed

api test Build finished. 351 tests run, 4 skipped, 3 failed.
Details
framework test Build finished. No test results found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
integration test Build finished. 79 tests run, 4 skipped, 0 failed.
Details
selenium test Build finished. 118 tests run, 2 skipped, 0 failed.
Details
toolshed test Build finished. 577 tests run, 0 skipped, 0 failed.
Details
@dannon

This comment has been minimized.

Member

dannon commented Feb 15, 2018

@davebx Want to open a flattened backport of this?

@martenson martenson modified the milestones: 18.05, 18.01 Feb 15, 2018

@davebx davebx deleted the davebx:close_temp_files branch Feb 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment