file descriptor leak in filestore.retrieve()

[sascha_silbe]

filestore.retrieve() contains a file descriptor leak:

        attempt = 1
        while os.path.exists(destination_path):
            if attempt > 10:
                fd_, destination_path = tempfile.mkstemp(prefix=uid,
                                                         suffix=extension,
                                                         dir=destination_dir)
                del fd_
                os.unlink(destination_path)
                break
            else:
                file_name = '%s_%s%s' % (uid, attempt, extension)
                destination_path = os.path.join(destination_dir, file_name)
                attempt += 1

Since fd_ is a plain integer (i.e. not a file object), "del fd_" will not close the file descriptor, but leave it open.

What's the reason for having two different ways of choosing names, BTW? Why don't we just use a single mkstemp() call and be done with it?

[sascha_silbe]

Fix file descriptor leak in filestore.retrieve(), minimally invasive version

[sascha_silbe]

I've attached two possible fixes, a minimally invasive one and another that removes the loop and relies entirely on mkstemp().
Personally I prefer the second one as it eliminates the possibility of triggering bugs on rare occasions (i.e. only if there are already more than ten checked-out copies of the entry lying around).

[sascha_silbe]

Fix file descriptor leak in filestore.retrieve() and use only mkstemp()

[sascha_silbe]

Please review.

[alsroot]

not sure but just in case, could you push 1st patch to sucrose-0.84/6 branches and 2nd one to the master

and for 2nd patch, better to preserve previous naming scheme, it was "%s_%s%s" (just in case)

[tomeu]

This one is on risk of being lost in trac and never applied. Maybe we should have a report to detect reviewed patches that weren't pushed yet?

[tomeu]

ping

[alsroot]

sucrose-0.84
​http://git.sugarlabs.org/projects/sugar-datastore/repos/mainline/commits/71f000a02583edd0cac5a59b3228204c49f8412a

sucrose-0.86
​http://git.sugarlabs.org/projects/sugar-datastore/repos/mainline/commits/6e01fcbbb01a4d4f412286f98f18c19172977a49

master
​http://git.sugarlabs.org/projects/sugar-datastore/repos/mainline/commits/21c8e9c079bbe32a00a298dceecb551a3fbb1b56
​http://git.sugarlabs.org/projects/sugar-datastore/repos/mainline/commits/ba0a546d53bb10aa8540f9725488c8f7d0dde250

[dnarvaez]

Milestone 0.88 deleted