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

AttributeError in Popen.__del__ #63221

Closed
serhiy-storchaka opened this issue Sep 14, 2013 · 25 comments
Closed

AttributeError in Popen.__del__ #63221

serhiy-storchaka opened this issue Sep 14, 2013 · 25 comments
Labels
release-blocker stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@serhiy-storchaka
Copy link
Member

BPO 19021
Nosy @warsaw, @terryjreedy, @doko42, @pitrou, @vstinner, @larryhastings, @serhiy-storchaka
Files
  • subprocess_del_getattr.patch
  • subprocess_active_pids.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2014-03-07.11:04:31.634>
    created_at = <Date 2013-09-14.23:46:46.679>
    labels = ['type-bug', 'library', 'release-blocker']
    title = 'AttributeError in Popen.__del__'
    updated_at = <Date 2014-03-07.11:12:28.296>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2014-03-07.11:12:28.296>
    actor = 'Arfrever'
    assignee = 'none'
    closed = True
    closed_date = <Date 2014-03-07.11:04:31.634>
    closer = 'larry'
    components = ['Library (Lib)']
    creation = <Date 2013-09-14.23:46:46.679>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = ['31762', '34021']
    hgrepos = []
    issue_num = 19021
    keywords = ['patch']
    message_count = 25.0
    messages = ['197737', '197750', '198240', '198247', '198264', '198389', '198392', '198404', '198413', '198861', '198893', '198897', '210834', '212359', '212361', '212369', '212375', '212376', '212381', '212439', '212441', '212475', '212477', '212817', '212875']
    nosy_count = 10.0
    nosy_names = ['barry', 'terry.reedy', 'doko', 'pitrou', 'vstinner', 'larry', 'Arfrever', 'chortos', 'sbt', 'serhiy.storchaka']
    pr_nums = []
    priority = 'release blocker'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue19021'
    versions = ['Python 3.4']

    @serhiy-storchaka
    Copy link
    Member Author

    Sometimes closing IDLE I got such exception:

    Exception ignored in: <bound method Popen.__del__ of <subprocess.Popen object at 0xb5b8618c>>
    Traceback (most recent call last):
      File "/home/serhiy/py/cpython/Lib/subprocess.py", line 890, in __del__
    TypeError: 'NoneType' object is not callable

    It is happened because the builtins module was destroyed before running __del__ in process of garbage collecting.

    Here is a patch which should fix the bug. Anothe possible solution is get rid from getattr and catch AttributeError instead.

    See also bpo-16650.

    @serhiy-storchaka serhiy-storchaka added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Sep 14, 2013
    @terryjreedy
    Copy link
    Member

    The condition was changed from self._child_created to the getattr call in bpo-12085. As explained in msg197748, I think that change should be reverted (and this issue closed). I think elaborating a bad patch only makes it worse.

    @chortos
    Copy link
    Mannequin

    chortos mannequin commented Sep 22, 2013

    Anothe possible solution is get rid from getattr and catch
    AttributeError instead.

    Surely this would suffer from the same issue?

    Why are the builtins getting deleted anyway? In fact, why is getattr getting deleted from the builtins module? The __builtins__ global is specifically not deleted when deleting modules to let destructors use builtins, and indeed the error message says "is not callable", suggesting the builtins module is still there but getattr is not. Is this perhaps some sort of IDLE bug?

    @terryjreedy
    Copy link
    Member

    When the patch to bpo-12085 is changed, as has been agreed, I think, this issue should go away.

    Moving the deletion of builtins to later in the shutdown process has be discussed and maybe implemented.

    @chortos
    Copy link
    Mannequin

    chortos mannequin commented Sep 22, 2013

    But the thing is, builtins are already supposed to be the very last thing destroyed at shutdown.

    @serhiy-storchaka
    Copy link
    Member Author

    I'm wondering too. While the code in Popen.__del__() is almost same, I can't reproduce the issue in 3.3. Perhaps it relates to some 3.4 changes? Import machinery, weak references, the shutdown process?

    Before applying the patch which fixes Popen.__del__() I want understand what happens.

    @sbt
    Copy link
    Mannequin

    sbt mannequin commented Sep 25, 2013

    The clearing of modules at shutdown has been substantially changed in 3.4. Now a best effort is made to let the module go away purely by gc. Those modules which survive get purged in random order.

    In 3.3 all modules were purged, but builtins was special cased to be purged last. (See Python/import.c:PyImport_Cleanup().)

    I would favour setting a flag before the purging stage which prevents __del__ methods (and weakrefs?) from running.

    @serhiy-storchaka
    Copy link
    Member Author

    Could we instead restore the builtins module to it's initial state before the purging stage? I believe all builtins are immutable and can't create reference loops.

    @terryjreedy
    Copy link
    Member

    Before applying the patch which fixes Popen.__del__()

    I think your bpo-12085 patch should be applied and that issue closed. It is not specifically about shutdown. Tweaking shutdown further would be a new issue.

    @serhiy-storchaka
    Copy link
    Member Author

    There is a regression in 3.4 due to changes in shutdown procedure. This code correctly works in 3.3. There are more than a dozen places in the stdlib which rely upon accessibility of builtins. I wrote patches for all these cases, but third-party code will be broken.

    I think we should restore guarantees about builtins.

    @pitrou
    Copy link
    Member

    pitrou commented Oct 3, 2013

    There is a regression in 3.4 due to changes in shutdown procedure. This
    code correctly works in 3.3. There are more than a dozen places in the
    stdlib which rely upon accessibility of builtins.

    Well, perhaps we can special-case builtins not to be "wiped" at shutdown.
    However, there is another problem here in that the Popen object survives until the builtins module is wiped. This should be investigated too.

    @sbt
    Copy link
    Mannequin

    sbt mannequin commented Oct 3, 2013

    Well, perhaps we can special-case builtins not to be "wiped" at shutdown.
    However, there is another problem here in that the Popen object survives
    until the builtins module is wiped. This should be investigated too.

    Maybe it is because it uses the evil resuscitate-in-del trick. I
    presume that if the child process survives during shutdown, then the popen
    object is guaranteed to survive too.

    We could get rid of the trick:

    • On Windows __del__ is unneeded since we don't need to reap zombie
      processes.

    • On Unix __del__ could just add self._pid (rather than self) to the list
      _active. _cleanup() would then use os.waitpid() to check the pids in
      _active.

    The hardest thing about making such a change is that test_subprocess
    currently uses _active.

    @serhiy-storchaka
    Copy link
    Member Author

    Here is a patch which implements Richard's suggestion: _active now contains pid-s instead of Popen instances. But this doesn't fix this issue. Patches for bpo-19255 and bpo-12085 fixes it.

    @warsaw
    Copy link
    Member

    warsaw commented Feb 27, 2014

    We're seeing this in Ubuntu now that 3.4 is the default.

    https://bugs.launchpad.net/python/+bug/1284469

    @vstinner
    Copy link
    Member

    Serhiy fixed the issue bpo-19255 with the changeset 6a1711c96fa6, but this changeset will wait Python 3.4.1.

    @warsaw
    Copy link
    Member

    warsaw commented Feb 27, 2014

    On Feb 27, 2014, at 03:46 PM, STINNER Victor wrote:

    Serhiy fixed the issue bpo-19255 with the changeset 6a1711c96fa6, but this
    changeset will wait Python 3.4.1.

    Okay, thanks. I was reviewing and rather liked the less invasive patch, but
    if this one is going to make it into 3.4.1, I'll review it and test it
    locally. We can patch Ubuntu's 3.4 for now.

    @warsaw
    Copy link
    Member

    warsaw commented Feb 27, 2014

    If this fixes the problem, shouldn't the issue be closed and a NEWS item added? I'm going to test the patch locally.

    @pitrou
    Copy link
    Member

    pitrou commented Feb 27, 2014

    Hmm... if this *already* affects Ubuntu users, shouldn't this be fixed in 3.4 proper? It's extremely likely that someone else will be affected too.

    @warsaw
    Copy link
    Member

    warsaw commented Feb 27, 2014

    On Feb 27, 2014, at 07:23 PM, Antoine Pitrou wrote:

    Hmm... if this *already* affects Ubuntu users, shouldn't this be fixed in 3.4
    proper? It's extremely likely that someone else will be affected too.

    Agreed!

    @warsaw
    Copy link
    Member

    warsaw commented Feb 28, 2014

    I've testing this patch on Ubuntu, and it seems to fix the problem. My quick testing doesn't show any new problems, but we'll only know for sure once the new Python 3.4 package hits the archive and folks start updating to it. So far so good though.

    Larry, please consider cherry picking this into 3.4.0 final.

    @warsaw
    Copy link
    Member

    warsaw commented Feb 28, 2014

    Nosying Doko, since I think he may want to get this fix into Debian, if Larry does not cherry pick it into 3.4.0 final.

    @serhiy-storchaka
    Copy link
    Member Author

    Note that the changeset 6a1711c96fa6 introdused several errors (mostly tests failures), so it needs other changesets to fix it. Related changesets which should be cherry picked: 6a1711c96fa6, fa160c8145e5, efaf12106d68, 7ecee9e0dc58, 10ea3125d7b8, 488ccbee6ee6.

    @warsaw
    Copy link
    Member

    warsaw commented Feb 28, 2014

    On Feb 28, 2014, at 09:42 PM, Serhiy Storchaka wrote:

    Note that the changeset 6a1711c96fa6 introdused several errors (mostly tests
    failures), so it needs other changesets to fix it. Related changesets which
    should be cherry picked: 6a1711c96fa6, fa160c8145e5, efaf12106d68,
    7ecee9e0dc58, 10ea3125d7b8, 488ccbee6ee6.

    Interestingly enough, the test failures don't seem to break the build, but do
    seem to break the "DEP 8" tests, which are run on the built package and must
    pass before the package is promoted to the primary archive. It takes quite a
    while to build Python for Ubuntu (since we do several builds, e.g. debug
    builds, etc. along with all the tests), so I've just noticed this.

    Thanks for listing the additional revisions that need to be cherry picked. I
    will investigate for Ubuntu, but it does convince me more that Larry should
    attempt to pull these into 3.4.0.

    @larryhastings
    Copy link
    Contributor

    So you're asking that I cherry pick six revisions here?

    6a1711c96fa6
    fa160c8145e5
    efaf12106d68
    7ecee9e0dc58
    10ea3125d7b8
    488ccbee6ee6

    @larryhastings
    Copy link
    Contributor

    Those six revisions have been cherry-picked into 3.4.0.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    release-blocker stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants