Change Request -- high risk, high reward

[Toshio Kuratomi]

mmcgrath at redhat.com wrote:
> 
> On Mar 14, 2009, at 8:27 PM, Nigel Jones <nigjones at redhat.com> wrote:
> 
>>
>> ----- "Toshio Kuratomi" <a.badger at gmail.com> wrote:
>>
>>> ricky and I have identified a piece of code in fas2's new
>>> safasprovider
>>> that's querying the database a lot.  It's causing anything that
>>> checks
>>> identity to issue a query to the database to lookup the visit cookie.
>>> This is causing things that lookup information on the identity
>>> multiple
>>> times to take a lot of time.  One of the functions that does this is
>>> filter_private(), the function that removes excess information
>>> according
>>> to privacy settings and who is looking for the data.  Our test was
>>> the
>>> /user/list method which is currently running over 5 minutes for an
>>> otherwise non-database loop.  Changing this caused the loop to run for
>>> 8
>>> seconds.
>>>
>>> That's the benefit.  The risk is that this is a change to the
>>> safasprovider, ie the portion of fas2 that authenticates the user.
>>> So
>>> if there's a reason this shouldn't be cached, we could potentially be
>>> breaking a lot of things.
>>>
>>> Ricky and I have both looked at the code in
>>> fas/safasprovider.py::SaFasIdentity and think that it's safe to cache
>>> this.  The TG-1.0.8 saprovider on which safasprovider is based does
>>> not
>>> cache this but I've looked at the code and it seems like their
>>> provider
>>> only uses the variable in question a maximum of two times during a
>>> request.  The CSRF protection that we've enabled needs to use this
>>> variable more often.
>>>
>>> Here's the code:
>>>
>>> --- a/fas/safasprovider.py
>>> +++ b/fas/safasprovider.py
>>> @@ -65,6 +65,7 @@ class SaFasIdentity(object):
>>>
>>>     def __init__(self, visit_key=None, user=None, using_ssl=False):
>>>         self.visit_key = visit_key
>>> +        self._visit_link = None
>>>         if user:
>>>             self._user = user
>>>             if visit_key is not None:
>>> @@ -201,9 +202,13 @@ class SaFasIdentity(object):
>>>     ### TG: Same as TG-1.0.8
>>>     def _get_visit_link(self):
>>>         '''Get the visit link to this identity.'''
>>> +        if self._visit_link:
>>> +            return self.visit_link
>>>         if self.visit_key is None:
>>> -            return None
>>> -        return
>>> visit_class.query.filter_by(visit_key=self.visit_key).first()
>>> +            self._visit_link = None
>>> +        else:
>>> +            self._visit_link =
>>> visit_class.query.filter_by(visit_key=self.visi
>>> t_key).first()
>>> +        return self._visit_link
>>>     visit_link = property(_get_visit_link)
>>>
>>> If we were outside of freeze, I would apply this as it is causing
>>> issues
>>> for some of the things that talk to fas (like zodbot and developer
>>> instances of pkgdb).
>>>
>>> -Toshio
>> +1 - I live for danger!
>>
>> That said, anything that speeds it up is a good thing!
> 
> +1. Just the beta freezeand the revert is easy.
> 
Thanks guys.  Hotfixed on the server.  If anyone notices wierdness with
authentication to fas, let me know.

-Toshio

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/fedora-infrastructure-list/attachments/20090314/21a7088f/attachment.sig>