Roan Kattouw <roan.katt...@gmail.com> changed:
What |Removed |Added
--- Comment #4 from Roan Kattouw <roan.katt...@gmail.com> 2010-12-08 15:06:49
(In reply to comment #3)
> Okay, there's a number of things here...
> Somewhat irrelevant to the main thrust here -- making the "local" cache of the
> file in UploadStash->files upon storage, rather than a memoized retrieval --
> this seems wrong to me as you only get a chance to do this in the same process
> as the one that created the record. Any subsequent access is slow. If the
> argument is that the caches are fast anyway then the whole optimization of
> having UploadStash->files should be removed.
I don't see the issue here. UploadStash->$files is not shared between processes
or anything. Memoization only works within the same process. AFAICT this
revision changed nothing in this regard: it writes to $files upon storage AND
upon retrieval (before, we only wrote upon retrieval but did a retrieval at the
end of the storage function; hitting $_SESSION twice like that is fine, hitting
the cache unnecessarily is not, that's why I refactored this a little bit)
> Okay, the main thing you did is to:
> - make entries directly in cache. I verified that there isn't any case in
> MediaWiki where we have absolutely no cache available, so that seems okay.
> - but now we're relying on an unspecified number of forms of cache, that will
> change in the future, to do intraserver communication. Unlike $_SESSION which
> has to do that even for a website to work, other forms of caching may not.
> will happen to work in the MediaWiki cluster since we'll get memcache, and on
> typical standalone MediaWiki you'll get a *DBM, so maybe it's okay, but it
> bothers me a little.
This is a good point. I guess we could do what I was considering initially,
which is to choose between the old and the "new" behavior based on a $wg var
which, if unset, would default to equal $wgSessionsInMemcached. The rationale
here is that if sessions are in memcached, the new way is definitely safe (just
as safe as $_SESSION would be, anyway) and even required (because there's no
session locking in this case). Cases where $wgSessionsInMemcached is disabled
but session locking is somehow disabled too are probably very rare.
> - Since you give each upload their own key, they are truly isolated so that
> probably will fix the concurrency issue. This makes all the uploads truly
> isolated, which is nice, but also breaks with the convention of storing all of
> them under a particular key (this is what everyone else does, such as the
> UploadByUrl, Firefogg uploads) and it also makes it difficult to find all the
> uploads later if we wanted to. There is no case in the application where we
> have to do this, but I anticipate we will want to do this later.
Yeah you can't do array_keys( $_SESSION['wsUploadData'] ) to list your
currently stashed uploads anymore, that's right. This is by design, however:
having all the keys in one place like that is what caused these concurrency
issues in the first place.
> - It might be as simple as eliminating the line that creates an array in
> $_SESSION[UploadBase::SESSION_NAME]. This is PHP, multi-dimensional hashes
> spring into existence on assignment. Depending on how this is implemented
> that solves it.
I'm pessimistic about this: I think the session handler writes the data to
memcached when the request ends, in which case it doesn't matter how that data
was put in $_SESSION exactly.
> - Actually use the database -- least desirable from a standpoint of doing a
> quick fix, but probably the best long term solution
This is probably a good idea, as it gives us excellent concurrency control;
expiry wouldn't be handled, though, so we'd have to do that ourselves the way
Block.php and SqlBagOStuff (best class name ever) already do.
Configure bugmail: https://bugzilla.wikimedia.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.
Wikibugs-l mailing list