Re: [gentoo-portage-dev] Plugin backport PATCH (1/2)/(2/2)

[Brian Harring]

On Sat, Nov 19, 2005 at 01:12:46PM +0900, Jason Stubbs wrote:
> > > +++ ./pym/portage_locks.py      2005-11-16 01:56:25.152161768 -0500
> > > @@ -358,3 +359,91 @@
> > > +# should the fd be left open indefinitely?
> > > +# IMO, it shouldn't, but opening/closing everytime around is expensive
> > >
> > > It should be closed when locks are released, I think. Otherwise the file may
> > > be deleted and recreated while one FsLock instance has it open which could
> > > create situations where two FsLocks are trying to lock the same path but
> > > using different fds.
> > Leave it open, on locking do a inode verification of the existing
> > handle- if it's behind, close the fd, open, resume locking (offhand).
> >
> > Race potential, but that can be addressed by getting the read lock,
> > then doing a second (or first, if it's not re-upping a lock) inode
> > verification
>
> Umm.. getting complicated. How often are locks actually obtained, released
> and then obtained a second time? Sacrificing readability for performance in
> corner cases doesn't seem quite right.
It's not really that complicated, and (imo) is a requisite if the 
locking obj is used for synchronizing during deletions; 

lock_obtained = False
while not lock_obtained:
	if self.fd:
		self.read_lock()
		# check that the file didn't change under us
		if os.fstat(self.fd).st_ino != os.stat(self.path).st_ino):
			self.release_lock()
		else:
			lock_obtained = True
	
	if not self.fd:
		self._acquire_fd()

Would be a rough example of it, bordering pseudocode (pardon bugs, 
5:48am my time right now). :)

EIther way, yes, it's a bit complicated, but locking isn't simple; as 
long as the api exported by the object is simple, handling the nasty 
crap internally shouldn't be an issue.


> Also, with a long running portage
> process and badly written code, it'd be quite easy to run out of fds...
All locking code (stable included) has this potential though- it's not 
something limited to this implementation.

> > > +       def __del__(self):
> > > +               # alright, it's 5:45am, yes this is weird code.
> > > +               try:
> > > +                       if self.fd != None:
> > > +                               self.release_read_lock()
> > > +               finally:
> > > +                       if self.fd != None:
> > > +                               os.close(self.fd)
> > >
> > > Allowing an exception to be raised during __del__ is not a good thing...
> >
> No traceback though... Something like the following (on one line):
>
> Exception exceptions.Exception:
> <exceptions.Exception instance at 0x2aaaaab2db48> in
> <bound method foo.__del__ of <__main__.foo instance at 0x2aaaaab2d5f0>>
> ignored
>
> I guess it's enough to point out where debugging needs to start...
Exactly.  Misbehaving __del__ code can be fun to track, usually prefer 
the ugly complaints dumped to term to silence.

> > > +++ ./pym/portage_modules.py    2005-11-16 01:56:28.269687832 -0500
> > > +               # * ferringb|afk hit it at some point, sure of that
> > > +               # but no idea how, so commenting out to see if things break...
> > > +               # except AttributeError, e:
> > > +               #       raise FailedImport(name, e)
> > >
> > > I can't see how either, but comments like these don't really help. Please
> > > leave them out unless they describe how the code works.
> > Leaving comments regarding "weirdness occured via here, but went away"
> > isn't a bad thing- if it reappears, you know that at least it was hit
> > once prior.
> >
> > I know how it was hit also (just dawned on me how it would happen)-
> > echo $'import os;print os.dar' > test.py
> > python -c'import test'
> >
> > AttributeError...
> >
> > Should nuke the module imo in that case still, since it's a failed
> > import.
>
> Yep, so it shouldn't be commented then - which is my point. If code is to be
> going into stable, things like this should be resolved prior to.

Code needs further work if it's going into stable imo.

Reason I wanted the code split off from 3.0 and backported was to show 
a file based approach rather then trying to do scans at load up.


> > > +       def query_plugins(self, plugin_type=None, locking=True, raw=False):
> > >
> > > Not sure about allowing the external callers to disable locking... That
> > > should only be used by register and deregister, right? Perhaps breaking
> > > the unlocked version into a "protected" method?
> >
> > Debatable; any protection used people can stomp right past.  Not much
> > for the "sitting on the porch with the shotgun" approach.
> >
> > Not really seeing the gain in hiding the locking tbh; shoving it into
> > the instance namespace would require locking the attribute to maintain
> > thread safety; the only gain is just an extra hoop to jump through if
> > I'm after doing something stupid. :)
>
> Should query_plugins be called without no locking in place? In the code so
> far it is only called when a lock is being maintained outside of the method
> call. I'm not sure what you are referring to with regard to instance
> namespace. To be clear, I'm suggesting:
>
> def register_plugins(...):
> 	lock_stuff()
> 	do_stuff()
> 	internal_query_plugins()
> 	unlock_stuff()
>
> def query_plugins(...):
> 	lock_stuff()
> 	internal_query_plugins()
> 	unlock_stuff()
>
> def internal_query_plugins(...):
> 	do_stuff()
>
> There's no namespace stuff there...
Doable in that route; was assuming you meant to shove the lock into 
the instance.

> > > +                                       d[x[:-len_exten]] = dict([(y, dict(c.items(y))) for y in c.sections()])
> > >
> > > Same here on the 2.2 bit... Although that's pretty hard to read either way.
> > Prefer a map call? :)
>
> Whatever is most readable (and supported).
Bit from above is supported in 2.2 and up.  Probably early, offhand.
The example is a bit out of context, since the len_exten is defined 
above, and clear what it's doing- the dict arg instantiation might be 
a bit weird if people don't know that you can (essentially) pass a 
zipped set of arrays to dict, which it'll convert into key->vals... 
that said, chunking out a replacement of 3 lines seems a bit pointless 
imo.

Dunno.  for stable, can go either way, for 3.0, questionable since 
it's a common enough trick.

> > > +               finally:
> > > +                       if locking:
> > > +                               plug_lock.release_read_lock()
> > > +               if plugin_type != None:
> > > +                       return d[plugin_type]
> > > +               return d
> > >
> > > Not sure about having different return types...
> > Agreed.
>
> So one method for querying what plugin types are available and another for
> querying plugins of a given type?
Or one for querying the full tree, one for pulling a specific 
branch...

> > > +registry = None
> > > +from portage.util.currying import pre_curry, pretty_docs
> > > +
> > > +def proxy_it(method, *a, **kw):
> > > +       global registry
> > > +       if registry == None:
> > > +               registry = GlobalPluginRegistry()
> > > +       return getattr(registry, method)(*a, **kw)
> > > +
> > > +for x in ["register", "deregister", "query_plugins", "get_plugin"]:
> > > +       v = pre_curry(proxy_it, x)
> > > +       doc = getattr(GlobalPluginRegistry, x).__doc__
> > > +       if doc == None:
> > > +               doc = ''
> > > +       else:
> > > +               # do this so indentation on pydoc __doc__ is sane
> > > +               doc = "\n".join(map(lambda x:x.lstrip(), doc.split("\n"))) +"\n"
> > > +       doc += "proxied call to module level registry instances %s method" % x
> > > +       globals()[x] = pretty_docs(v, doc)
> > > +
> > > +del x, v, proxy_it, doc
> > >
> > > What's this currying and pretty_docs stuff?
> > currying == arg binding to a func- in this case, proxy_it gets first
> > arg of the attr requested; the proxy_it func instantiates a
> > GlobalPluginRegistry instance if it's missing.
>
> This stuff seems to be missing from the patch then.
Offhand, looking over the split patch, this won't even work.
The from portage.util.currying import ... is still savior namespace.

Curious, qualms about using a massively cleaned up version of this?  
The reason I wanted this seperated out and backported was to go with 
an actual registry, and abuse the existing code- specific complaints 
with that route, beyond patch sucking?
~harring