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



Copy link

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 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 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 is often the wanted command, especially for reading metadata.

…ring file handle limits
Copy link

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.

Copy link

planetmaker commented Dec 16, 2019

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:

Copy link

LordAro left a comment

To reflect above comments

Copy link
Contributor Author

andythenorth commented Apr 20, 2020

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 added a commit that referenced this pull request Apr 20, 2020
Change: Convert to open().
Copy link

LordAro commented Apr 20, 2020


@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
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.