On Wed, Oct 19, 2011 at 12:55 PM, Happy Melon happy.melon.wiki@gmail.comwrote:
On 19 October 2011 20:41, Russell Nelson russnelson@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@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