On Wed, Oct 19, 2011 at 12:55 PM, Happy Melon <happy.melon.wiki(a)gmail.com>wrote;wrote:
On 19 October 2011 20:41, Russell Nelson
<russnelson(a)gmail.com> wrote:
In order to support viewing deleted files
(currently a blocker bug in
SwiftMedia), I'm going to refactor File::getPath() into a new public
function File::getLocalPath(), which will return an instance of a new
class TempLocalPath, which will have two methods: getPath(), and
close(). This class will own a local copy of the file. When it goes
out of scope or its close() method is called (same thing), any
resources held by the class will be freed.
Is there a more standardised name than "close"? "dispose" is pretty
common
in compiled languages; do we have any sort of standard for that behaviour
within MW? If not, is this a good opportunity to create one?
We have free() on things like database result wrappers; I'd probably stick
with that. Generally though nobody has to call these explicitly as the
runtime's reference counting auto-destroys objects once they fall out of
use, and the destructor calls it for us.
As Tim noted, the particular case in Special:Undelete is probably an example
of old code using a lazy interface due to old assumptions, and should
actually be changed to use a proper File reference, and have a method to
call from the File object / via the FileRepo/FileBackend that wraps
streaming output so it doesn't actually need to do a file checkout.
This didn't come up as much for ForeignAPIRepo because we do fewer direct
operations on files from non-local repositories: we leave metadata lookups
and thumbnailing to the foreign site, so don't have to pass the file into
MediaHandler's shell-outs. We don't stream restricted or deleted files out
because we don't manage their deletion/undeletion or security.
On Wed, Oct 19, 2011 at 3:06 PM, Platonides <Platonides(a)gmail.com> wrote:
Why is this needed?
A FileSwift class which needed to fetch the file from Swift could
acquire it when calling getPath(), and have it deleted on FileSwift
destruction (getPath() is usually called from several methods, so it
isn't practical to acquire and destroy every time).
I would broadly warn against unnecessarily checking files out to a local
filesystem: while this is fairly cheap with tiny JPEGs and PNG diagrams, and
feels cheap on modern computers for several-megabyte photos, it's still
potentially very expensive for multi-hundred-meg and multi-gigabyte video
files.
Any time we are copying to a local file, we should be sure that we *really
mean it*.
A getPath() that implicitly copies files makes me uneasy... I might actually
recommend putting a deprecation warning on it to help find and reduce these.
A more explicit check-out / discard interface will let us keep existing code
like our thumbnailing that does need to deal with local files, without
hiding that it's a potentially expensive operation. And that can encourage
us to move things towards interfaces that don't need to check files out, but
can read them as streams or skip expensive original-file checks in the first
place when they're not really needed.
-- brion