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

Make regrtest with --huntrleaks check for fd leaks #62374

Closed
sbt mannequin opened this issue Jun 9, 2013 · 25 comments
Closed

Make regrtest with --huntrleaks check for fd leaks #62374

sbt mannequin opened this issue Jun 9, 2013 · 25 comments
Labels
tests Tests in the Lib/test dir type-feature A feature request or enhancement

Comments

@sbt
Copy link
Mannequin

sbt mannequin commented Jun 9, 2013

BPO 18174
Nosy @pitrou, @vstinner, @ezio-melotti
PRs
  • [2.7] bpo-18174: Fix file descriptor leaks in tests #7408
  • [2.7] bpo-18174: regrtest -R 3:3 now also detects FD leak #7409
  • [2.7] bpo-18174: Fix fd_count() on FreeBSD #7420
  • bpo-18174: regrtest -R 3:3 checks for handle leak #7827
  • bpo-33966, multiprocessing: Pool wait for worker start #7966
  • Files
  • fdleak.patch
  • fdleak.patch
  • fdleak-2.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 2018-09-19.22:48:53.733>
    created_at = <Date 2013-06-09.19:47:53.846>
    labels = ['type-feature', 'tests']
    title = 'Make regrtest with --huntrleaks check for fd leaks'
    updated_at = <Date 2018-09-19.22:48:53.732>
    user = 'https://bugs.python.org/sbt'

    bugs.python.org fields:

    activity = <Date 2018-09-19.22:48:53.732>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-09-19.22:48:53.733>
    closer = 'vstinner'
    components = ['Tests']
    creation = <Date 2013-06-09.19:47:53.846>
    creator = 'sbt'
    dependencies = []
    files = ['30518', '30561', '36147']
    hgrepos = []
    issue_num = 18174
    keywords = ['patch']
    message_count = 25.0
    messages = ['190871', '190915', '190918', '191031', '223790', '223874', '223881', '223882', '223885', '223887', '224192', '224200', '224201', '224203', '224236', '224239', '224241', '251849', '252169', '252172', '252183', '318720', '318732', '318735', '325815']
    nosy_count = 6.0
    nosy_names = ['pitrou', 'vstinner', 'ezio.melotti', 'neologix', 'python-dev', 'sbt']
    pr_nums = ['7408', '7409', '7420', '7827', '7966']
    priority = 'normal'
    resolution = 'wont fix'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue18174'
    versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

    @sbt
    Copy link
    Mannequin Author

    sbt mannequin commented Jun 9, 2013

    regrtest already tests for refcount leaks and memory allocation leaks. It can also be made to check for file descriptor leaks (and perhaps also handles on Windows).

    Running with the attached patch makes it look like test_openpty, test_shutil, test_subprocess, test_uuid all leak fds on Linux, but I have not investigated:

    $ ./python -m test.regrtest -R 3:3 test_openpty test_shutil test_subprocess test_uuid
    [1/4] test_openpty
    123456
    ......
    test_openpty leaked [2, 2, 2] fds, sum=6
    [2/4/1] test_shutil
    beginning 6 repetitions
    123456
    ......
    test_shutil leaked [4, 4, 4] fds, sum=12
    [3/4/2] test_subprocess
    beginning 6 repetitions
    123456
    ......
    test_subprocess leaked [5, 5, 5] fds, sum=15
    [4/4/3] test_uuid
    beginning 6 repetitions
    123456
    ......
    test_uuid leaked [1, 1, 1] fds, sum=3
    4 tests failed:
        test_openpty test_shutil test_subprocess test_uuid

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 10, 2013

    New changeset a7381fe515e8 by Richard Oudkerk in branch '2.7':
    Issue bpo-18174: Fix fd leaks in tests.
    http://hg.python.org/cpython/rev/a7381fe515e8

    New changeset 46fe1bb0723c by Richard Oudkerk in branch '3.3':
    Issue bpo-18174: Fix fd leaks in tests.
    http://hg.python.org/cpython/rev/46fe1bb0723c

    @sbt
    Copy link
    Mannequin Author

    sbt mannequin commented Jun 10, 2013

    The test_shutil leak is caused by bpo-17899. The others are fixed by a7381fe515e8 and 46fe1bb0723c.

    @sbt
    Copy link
    Mannequin Author

    sbt mannequin commented Jun 12, 2013

    Updated version which adds checks for handle leaks on Windows.

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Jul 23, 2014

    Just flagging this up as testing is rather important.

    @BreamoreBoy BreamoreBoy mannequin added the tests Tests in the Lib/test dir label Jul 23, 2014
    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Jul 24, 2014

    The patch looks good.
    I just think it would be nice to expose _fdcount() in test.support.

    @vstinner
    Copy link
    Member

    Why not using os.fstat() instead of os.dup() to check if a file descriptor is open or not?

    It's strange to create a new file descriptor with os.dup() to count the file descriptors.

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Jul 24, 2014

    Why not using os.fstat() instead of os.dup() to check if a file descriptor is open or not?

    I asked myself the same question, but IIRC, fstat() doesn't always
    work on Windows (of course).

    @vstinner
    Copy link
    Member

    I asked myself the same question, but IIRC, fstat() doesn't always
    work on Windows (of course).

    Can you please elaborate?

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Jul 24, 2014

    STINNER Victor added the comment:

    > I asked myself the same question, but IIRC, fstat() doesn't always
    work on Windows (of course).

    Can you please elaborate?

    Not really, since I don't know much about Windows, but that's
    something I think I heard.
    Richard will be able to give more details.

    @sbt
    Copy link
    Mannequin Author

    sbt mannequin commented Jul 28, 2014

    I can't remember why I did not use fstat() -- probably it did not occur to me.

    @vstinner
    Copy link
    Member

    Modified patch to use os.fstat(), and try to use /proc/self/fd/ on Linux.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 28, 2014

    New changeset 379aad232000 by Victor Stinner in branch '3.4':
    Issue bpo-11453, bpo-18174: Fix leak of file descriptor in test_asyncore
    http://hg.python.org/cpython/rev/379aad232000

    New changeset 0ced2d2325fb by Victor Stinner in branch 'default':
    (Merge 3.4) Issue bpo-11453, bpo-18174: Fix leak of file descriptor in test_asyncore
    http://hg.python.org/cpython/rev/0ced2d2325fb

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 28, 2014

    New changeset 746339776f19 by Victor Stinner in branch '3.4':
    Issue bpo-18174: Fix leak of file descriptor in test_tempfile
    http://hg.python.org/cpython/rev/746339776f19

    New changeset 017d701116d5 by Victor Stinner in branch 'default':
    (Merge 3.4) Issue bpo-18174: Fix leak of file descriptor in test_tempfile
    http://hg.python.org/cpython/rev/017d701116d5

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Jul 29, 2014

    Richard Oudkerk added the comment:

    I can't remember why I did not use fstat() -- probably it did not occur to me.

    I probably have Alzeihmer, I was sure I heard a reasonable case for
    dup() vs fstat().
    The only thing I can think of is that fstat() can incur I/O, whereas
    dup() shouldn't.

    In any case, I too prefer fstat(), since it doesn't require creating a
    new FD (which could fail with EMFILE/ENFILE), and is more logical.

    The only comment I have about this patch is I think that the helper
    function to detect the number of open FD might have its place in
    test.support.

    @pitrou
    Copy link
    Member

    pitrou commented Jul 29, 2014

    fstat() can do I/O, or can fail for other reasons. If you don't want to create a new fd, I think you can do dup2(fd, fd).

    I don't understand the reason for the following code:

    + def check_handle_deltas(deltas):
    + return abs(sum(deltas)) >= min(3, len(deltas))

    Can you add a comment?

    @vstinner
    Copy link
    Member

    fstat() can do I/O, or can fail for other reasons.

    Oh, I forgot this reason. Maybe we should add a comment to explain that. I mean in the patch for this issue but also in is_valid_fd() (Python/pythonrun.c).

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 29, 2015

    New changeset 0e7d71a3bf0d by Victor Stinner in branch 'default':
    Issue bpo-18174: Explain why is_valid_fd() uses dup() instead of fstat()
    https://hg.python.org/cpython/rev/0e7d71a3bf0d

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 2, 2015

    New changeset 72129c767c92 by Victor Stinner in branch 'default':
    Issue bpo-18174: "python -m test --huntrleaks ..." now also checks for leak of
    https://hg.python.org/cpython/rev/72129c767c92

    @vstinner
    Copy link
    Member

    vstinner commented Oct 2, 2015

    I commited the first part to check for file descriptor leak.

    I didn't commit the second part, to check for Windows handle leak.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 3, 2015

    New changeset ec2ef7525fa5 by Victor Stinner in branch 'default':
    Issue bpo-18174: Fix test_regrtest when Python is compiled in release mode
    https://hg.python.org/cpython/rev/ec2ef7525fa5

    @ezio-melotti ezio-melotti added the type-feature A feature request or enhancement label Jan 1, 2016
    @vstinner
    Copy link
    Member

    vstinner commented Jun 4, 2018

    New changeset 2705819 by Victor Stinner in branch '2.7':
    bpo-18174: Fix file descriptor leaks in tests (GH-7408)
    2705819

    @vstinner
    Copy link
    Member

    vstinner commented Jun 5, 2018

    New changeset 64856ad by Victor Stinner in branch '2.7':
    bpo-18174: regrtest -R 3:3 now also detects FD leak (bpo-7409)
    64856ad

    @vstinner
    Copy link
    Member

    vstinner commented Jun 5, 2018

    New changeset bc3df70 by Victor Stinner in branch '2.7':
    bpo-18174: Fix fd_count() on FreeBSD (GH-7420)
    bc3df70

    @vstinner
    Copy link
    Member

    I give up on that close. I closed my PR 7827 and PR 7966.

    @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
    tests Tests in the Lib/test dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants