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: close image files after use during palette check #68

Closed

Conversation

andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Dec 1, 2019

With pypy, nmlc trivially trips file handle limits on macOS during palette checks, resulting in: "[Errno 24] Too many open files".

This is a common enough problem in python, which can be resolved by either

  • end user setting ulimit -n 4096 on a per shell basis
  • closing PIL Image files with Image.close(), where Image.open() has been used

This PR closes images opened for palette checks, which is a trivial case. Errno 24 no longer triggers for me. Tests pass.

Errno 24 does not trigger for me with any cPython 3.x, but pypy cuts run times by up to 50% in my testing compared to cPython 3.8, so we do want to be compatible with pypy.

There are a couple of other uses of Image.open() but they looked less trivial, and they're not tripping Errno 24 for me.

For the record, Image.load() self-closes so wouldn't have the same issue, but Image.open() is often the wanted command, especially for reading metadata.

@nielsmh
Copy link
Contributor

nielsmh commented Dec 1, 2019

I'm wondering if using a with block (context manager) isn't the more correct thing here? Assuming the Image class supports it, but I'd expect it to.

@andythenorth
Copy link
Contributor Author

@planetmaker
Copy link
Contributor

I agree with fsj: the closing should be done by means of using the with contextmanager
In good script kiddie manner, here's some examples:
https://stackoverflow.com/questions/713794/catching-an-exception-while-using-a-python-with-statement

Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

To reflect above comments

@andythenorth
Copy link
Contributor Author

Thanks for reviews. Bearing in mind that I am a clown-shoes python programmer who does not know how to use a contextmanager, current options are:

  • I learn how to implement a contextmanager in combination with try / except, which is maybe trivial, but I do not know how to do it (the 'try/except' makes it not a trivial use of 'with')
  • someone else adds the context manager
  • we ship my solution, and leave a note that there are better ways
  • we leave nmlc 'broken' on macOS with larger grfs, with a viable workaround of 'ulimit -n 4096'

Xaroth added a commit to Xaroth/nml that referenced this pull request Apr 20, 2020
LordAro pushed a commit that referenced this pull request Apr 20, 2020
@LordAro
Copy link
Member

LordAro commented Apr 20, 2020

Superseded.

@LordAro LordAro closed this Apr 20, 2020
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.

None yet

4 participants