On Mon, Jun 11, 2012 at 09:21:15AM -0700, Shawn Walker wrote:
> On 06/10/12 22:57, Edward Pilatowicz wrote:
> >On Tue, Jun 05, 2012 at 04:43:59PM -0700, Shawn Walker wrote:
> >>On 05/22/12 01:45, Edward Pilatowicz wrote:
> >>----------------------------------------
> >>src/modules/client/pkgremote.py:
> >>----------------------------------------
> >> lines 177-182, 231-236, 444-446, 469-471, 491-494, 503-508, 559-562:
> >> seems like these need to be try/finally to ensure lock gets
> >> released even if execution is interrupted
> >>
> >
> >i don't think so.
> >
> >first, the locks are all in memory process locks.
> >
> >second, all the code under locks has been written such that the only way
> >it will fail is due to a bug. almost all the code under locks is
> >actually simple variable assignments (the exceptions are Thread.start(),
> >assert calls, and debug messages if they are enabled). if any of these
> >operations fail we are not going to recover and the client is going to
> >die with a stack trace and exit, there by making process lock recovery
> >irrelevant.
>
> If a user hits ctrl+c at just the right point, we won't unlock, and
> that's not a bug, that's just interrupted execution. And I wouldn't
> expect a stack trace in most cases from that.
>
so i spent a while looking at the code again to make sure we're going to
handle exceptions (including ctrl+c) correctly.
to this end, i created a new src/tests/api/t_async_rpc.py test file that
tests different kinds of asynchronous rpc calls and errors at (like rpc
calls that raise exceptions, or rpc calls where the server is killed
mid-call, etc). these tests ensure that the basics of python signal and
exception handling in the async call paths is correct.
i also created a new AsyncCall() class in src/modules/misc.py that can
be used to make async calls via a new thread, and i use this class in
pkgremote.py and in t_async_rpc.py. this new class has been designed to
be simple and doesn't use any locking. using this new class in
pkgremote.py allowed me to simplify things and eliminate all the
locking. (since now we no longer have multiple threads accessing the
same variables). doing this eliminates the need to catch any exceptions
and should make the code much easier to understand and maintain.
these changes will be available in the next webrev that i send out
(which i'll do after i address danek's comments).
> >>----------------------------------------
> >>src/modules/misc2.py
> >>----------------------------------------
> >> lines 250, 514: I don't see where 'type' gets assigned, and I
> >>don't think we should have a variable named the same as a builtin
> >>function either.
> >>
> >
> >'type' doesn't get assigned anywhere. this is a reference to the builtin.
>
> Then I'm super-confused :-/
>
> I think there are more bugs here then:
>
> 250 if type(desc) == type:
>
> For exampleL
>
> >>> print type("foo")
> <type 'str'>
> >>> print type("foo") == type
> False
> >>> print type("foo") is type
> False
>
> In short, I'm struggling to figure out what this code is supposed to do.
>
the data descriptions format supported by json_{encode|decode} can
contain objects and types. this code is there to detect when the data
description contains a type. for example, in plandesc.py we have:
__state__desc = {
...
"_rm_aliases": { str: set() },
so when processing an _rm_aliases entry, the first thing we'll do is
determine it's type by doing the following:
---8<---
>>> type({ str: set() })
<type 'dict'>
---8<---
then we know we're dealing with a dictionary. next, we'll try to
determine the type of data used for keys in the dictionary, so we'll do:
---8<---
>>> type(str)
<type 'type'>
---8<---
and that's where this check applies. in this case the data description
already contains a type, so we don't want to invoke type() on it.
we could avoid this situation by not allowing types in the data
descriptions, but i figured that allowing 'str' in the data description
was more readable than requiring something like '""'. (it also means
that things like 'list' can be used in place of '[]', 'set' in place of
'set()', etc.) it just makes things easier for consumers of the interface.
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
