Opened 9 years ago

Closed 5 years ago

#6499 closed Cleanup/optimization (invalid)

Close open file descriptors on compression

Reported by: Bastian Kleineidam <calvin@…> Owned by: nobody
Component: Core (Other) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Close another file descriptor leak.

Attachments (2)

0046-Close-open-file-descriptors.patch (1.1 KB) - added by Bastian Kleineidam <calvin@…> 9 years ago.
6499-2.diff (595 bytes) - added by Claude Paroz 5 years ago.
Updated to current trunk

Download all attachments as: .zip

Change History (9)

Changed 9 years ago by Bastian Kleineidam <calvin@…>

comment:1 Changed 9 years ago by Malcolm Tredinnick

Firstly, unconditionally importing cStringIO isn't a good idea. It won't work with Jython.

Secondly, the file descriptor should be closed as soon as the zbuf variable goes out of scope, due to its reference count decreasing to zero. What is the situation where the file descriptor is leaking here?

comment:2 Changed 9 years ago by Bastian Kleineidam <calvin@…>

The cStringIO import was already there, so this might be good to put in a separate ticket, together with other Jython fixes probably. Thanks for the note though.

Every open file object will eventually be closed when the variable goes out of scope and the garbage collector frees it. The problem is that this is not guaranteed. The variable might be part of reference cycles, in which case the garbage collector will never free it, or the program terminates before running the garbage collection. Both cases might occur very seldom (if at all), but better be safe than sorry, since open file desriptors are especially annoying on Windows systems or network shares.

In python 2.5 you even can even use the new "with" statement to do this in a nicer way, but the compatible way is a try-finally wrapper.

comment:3 Changed 9 years ago by Jacob

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

comment:4 Changed 6 years ago by Julien Phalip

Needs tests: set
Severity: Normal
Type: Cleanup/optimization

Changed 5 years ago by Claude Paroz

Attachment: 6499-2.diff added

Updated to current trunk

comment:5 Changed 5 years ago by Claude Paroz

Component: UncategorizedCore (Other)
Easy pickings: unset
Needs tests: unset
Patch needs improvement: unset
UI/UX: unset

comment:6 Changed 5 years ago by Aymeric Augustin

I may be missing something, but we're writing to a StringIO, so there isn't a fd involved at all, let alone leaked.

comment:7 Changed 5 years ago by Aymeric Augustin

Resolution: invalid
Status: newclosed

Closing the GzipFile is useful because it flushes its buffers. Closing the StringIO isn't necessary; it will be garbage collected eventually.

I fail to see the point of the patch: there's no fd involved in this code, and even if zbuf were a fd, the patch wouldn't close it (see http://hg.python.org/cpython/file/91cbac92a665/Lib/gzip.py#l359).

Note: See TracTickets for help on using tickets.
Back to Top