On Wed, Oct 19, 2011 at 12:55 PM, Happy Melon <[email protected]>wrote:
> On 19 October 2011 20:41, Russell Nelson <[email protected]> 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 <[email protected]> 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 _______________________________________________ Wikitech-l mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/wikitech-l
