FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Don't abort on FT2Font weakref. by anntzer · Pull Request #9074 · matplotlib/matplotlib · GitHub

Repository navigation

Don't abort on FT2Font weakref. - #9074

Merged
tacaswell merged 1 commit into
matplotlib:masterfrom
anntzer:ft2font-weakref
Feb 11, 2018
Merged

tacaswell merged 1 commit into
matplotlib:masterfrom
anntzer:ft2font-weakref

Conversation

anntzer commented Aug 23, 2017

Copy link
Copy Markdown
Contributor

Currently,

import weakref, matplotlib.ft2font
weakref.ref(matplotlib.ft2font.FT2Font("/usr/share/fonts/TTF/DejaVuSans.ttf"))

aborts Py3 with

terminate called after throwing an instance of 'char const*'
Fatal Python error: Aborted

whereas on Py2 a normal TypeError: cannot create weak reference to 'matplotlib.ft2font.FT2Font' object is raised.

This is because the following happens:

  • Py3 sets the TypeError for failure to create a weakref.
  • The FT2Font object gets GC'd as nothing is referring to it (yes,
    creating a weakref to a temporary is a bit silly).
  • The FT2Font destructor calls close_file_callback, which calls
    mpl_PyFile_DupClose, which calls PyObject_AsFileDescriptor... which,
    on Py3, calls the fileno() method on the file object.
  • PyObject_AsFileDescriptor fails because the originally set TypeError
    has not been cleared, so mpl_PyFile_DupClose likewise fails, causing
    close_file_callback to throw a C++ exception.

Instead, carefully stash and restore the current exception state, both
in mpl_PyFile_DupClose, and likewise below, in mpl_PyFile_CloseFile (the
latter is needed because otherwise PyObject_CallMethod will clear the
exception, cf comment in CPython's Objects/abstract.c:

/* PyObject_Call() must not be called with an exception set,
because it may clear it (directly or indirectly) and so the
caller loses its exception */

Note that if an exception actually gets raised by mpl_PyFile_DupClose
or mpl_PyFile_CloseFile, it will overwrite the TypeError. Strictly
speaking, on Py3, it may be preferrable to chain the exceptions, but
this seems a bit overkill (mostly, I just want to avoid aborting the
whole Python process).

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

QuLogic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

A minor comment, but this works for me.

Comment thread src/file_compat.h Outdated
*/
static NPY_INLINE int mpl_PyFile_DupClose(PyObject *file, FILE *handle, mpl_off_t orig_pos)
{
PyObject *type = NULL, *value = NULL, *tb = NULL;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

These should probably get some differentiating name, like an orig_ prefix or something.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

grepping through https://github.com/python/cpython/blob/f320be77ffb73e3b9e7fc98c37b8df3975d84b40/Modules/_io/fileio.c shows that cpython tends to stash the current exception (if any) in *exc, *val, *tb (but not without a prefix). We're not shadowing anything anyways...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sure, but those are generally around one other call, whereas this one is around almost an entire function body.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

added exc_ prefix.

Comment thread src/file_compat.h

static NPY_INLINE int mpl_PyFile_CloseFile(PyObject *file)
{
PyObject *type, *value, *tb;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

... and here too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

as above

Currently,

    import weakref, matplotlib.ft2font
    weakref.ref(matplotlib.ft2font.FT2Font("/usr/share/fonts/TTF/DejaVuSans.ttf"))

aborts Py3 with

    terminate called after throwing an instance of 'char const*'
    Fatal Python error: Aborted

whereas on Py2 a normal `TypeError: cannot create weak reference to
'matplotlib.ft2font.FT2Font' object` is raised.

This is because the following happens:
- Py3 sets the TypeError for failure to create a weakref.
- The FT2Font object gets GC'd as nothing is referring to it (yes,
  creating a weakref to a temporary is a bit silly).
- The FT2Font destructor calls close_file_callback, which calls
  mpl_PyFile_DupClose, which calls PyObject_AsFileDescriptor... which,
  on Py3, calls the fileno() method on the file object.
- PyObject_AsFileDescriptor fails because the originally set TypeError
  has not been cleared, so mpl_PyFile_DupClose likewise fails, causing
  close_file_callback to throw a C++ exception.

Instead, carefully stash and restore the current exception state, both
in mpl_PyFile_DupClose, and likewise below, in mpl_PyFile_CloseFile (the
latter is needed because otherwise PyObject_CallMethod will clear the
exception, cf comment in CPython's Objects/abstract.c:

    /* PyObject_Call() must not be called with an exception set,
    because it may clear it (directly or indirectly) and so the
    caller loses its exception */

Note that if an exception *actually* gets raised by mpl_PyFile_DupClose
or mpl_PyFile_CloseFile, it will overwrite the TypeError.  Strictly
speaking, on Py3, it may be preferrable to chain the exceptions, but
this seems a bit overkill (mostly, I just want to avoid aborting the
whole Python process).
tacaswell merged commit 33bd5ed into matplotlib:master Feb 11, 2018
anntzer deleted the ft2font-weakref branch February 11, 2018 22:41
QuLogic modified the milestones: needs sorting, v2.2.0 Feb 12, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

4 participants


Back | FazBrowse Home | New Git URL