bzrlib.errors.IllegalUseOfScopeReplacer in pydoc call

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 8/28/2011 1:23 PM, Jelmer Vernooij wrote:
> ** Changed in: bzr Milestone: None => 2.5b1
>
> ** Changed in: bzr Status: In Progress => Fix Released
>

I'm guessing there are other locations that need this as well. Are you
sure it is just branch?

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5aJ6UACgkQJdeBCYSNAAOybACgqSLf/pnDmzMi51t+3Sxphv5R
szkAn2EtyEHlR4S3h/M5NS/B/QO2O21O
=MsTZ
-----END PGP SIGNATURE-----

On 28.08.2011 13:33, John A Meinel wrote:
> I'm guessing there are other locations that need this as well. Are you
> sure it is just branch?

There are others as well.

for i in $( find bzrlib \( -path bzrlib/tests -prune \) \
                      -o \( -name \*.py -print \) \
           | sed 's:\.py$::; s:/__init__$::; s:/:.:g;' ); do
  printf %-70s $i
  pydoc $i >/dev/null 2>/dev/null && printf 'OK\r' || printf 'FAIL\n'
done

This will give the following additional modules:

bzrlib.builtins
bzrlib.bundle
bzrlib.controldir
bzrlib.diff
bzrlib.osutils
bzrlib.plugins.weave_fmt.bzrdir
bzrlib.repofmt.knitpack_repo
bzrlib.repofmt.pack_repo
bzrlib.transport.local
bzrlib.vf_repository

Obviously the script does not check whether it's the same error. If it
is, is there a way to fix this in a single place instead of all those
modules? Don't really understand what the error message is about, I confess.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

...

> This will give the following additional modules:
>
> bzrlib.builtins bzrlib.bundle bzrlib.controldir bzrlib.diff
> bzrlib.osutils bzrlib.plugins.weave_fmt.bzrdir
> bzrlib.repofmt.knitpack_repo bzrlib.repofmt.pack_repo
> bzrlib.transport.local bzrlib.vf_repository
>
> Obviously the script does not check whether it's the same error. If
> it is, is there a way to fix this in a single place instead of all
> those modules? Don't really understand what the error message is
> about, I confess.
>

We use a lazy import mechanism to avoid loading code that we won't end
up using. To avoid runtime overhead, once the object has been called
or an attribute on it has been called, it replaces the object in the
scope. So doing:

lazy_import(globals(), """
from bzrlib import osutils
""")

def maybe_run():
  return osutils.foo()

If maybe_run gets called, when osutils.foo is accessed, it replaces
"osutils" in global scope with the real osutils module.

Because we are replacing the object intentionally, we explicitly
disable proxying the object. In the above scenario, if you did:

def maybe_run():
 ...

alt_osutils = osutils

When the "=" is evaluated at import time, 'osutils' is still the
ScopeReplacer object, and not the actual module. Which means that if
you ever did:

alt_osutils.foo()

It would have to act as a proxy (catching the getattr call, looking up
the real object, looking up the real attribute, calling it, returning
the result).

However, when pydoc runs, it touches everything in the module,
creating copies, but before they've had a chance to get 'replaced'
into the real objects.

I'd like to continue fixing these as we see them. Another option is to
change ScopeReplacer into supporting being a proxy, and turning it
into some sort of warning. However, we won't ever catch these warnings
in the test suite, because we don't run them in complete isolation
(you'd have to start a new python process for each test, and do all
the importing etc over and over again, which would take a couple
seconds *per test*.)

As this is annoying for developers, but not something that blocks
actual production code, I'm tending towards leaving ScopeReplacer as
it is.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5bWogACgkQJdeBCYSNAAMshACfb6q2PqZOuIoQg3X8a3QddX6w
v/8Ani9M4L5FAHms5QmuJnP9j/BxvYle
=IPCV
-----END PGP SIGNATURE-----

On 29.08.2011 11:23, John Arbash Meinel wrote:
> Because we are replacing the object intentionally, we explicitly
> disable proxying the object.

Thanks for the explanation.

How thread-safe is this approach? Suppose two threads concurrently
access the same replacer. Each grabs hold of a reference to the replacer
in the original scope entry. Then one thread continues, replaces the
object and cleans it up. The other thread might be just at the beginning
of __getattribute__ at that moment, perhaps past the _real_obj access.
So it won't proxy, even if proxying were enabled. Instead it will
encounter a cleaned replacer, thus raising the kind of exception we're
talking about here. Did I miss something?

> However, when pydoc runs, it touches everything in the module,
> creating copies, but before they've had a chance to get 'replaced'
> into the real objects.

I guess the internals of this "creating copies" is the reason why
importing modules is safe but importing classes is not, right?

> I'd like to continue fixing these as we see them.

As importing objects instead of modules appears to be quite common in
python, we should imho do something more about them than just fixing
them when whe see them.

One option that comes to my mind are doctests whether pydoc will work.
This requires spawning of a new python process for every module to test,
adding considerable time to the selftest suite. Would probably detect
other pydoc-related issues as well.

Another option would be actually disallowing lazy imports of anything
but modules, instead of just discouraging them. In other words, if the
ScopeReplacer (actually ImportReplacer, right?) ever gets replaced by
anything but a module, raise an exception then.

I guess the second approach would be the cleaner solution. Gave it a try
in lp:~gagern/bzr/lazy_import-force_modules .

-- Martin

> How thread-safe is this approach?

Yeah, it's not, see bug 396819 and <lp:~benji/bzr/bug-702914> which was recently merged on dev.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 8/29/2011 6:18 PM, Martin von Gagern wrote:
> On 29.08.2011 11:23, John Arbash Meinel wrote:
>> Because we are replacing the object intentionally, we explicitly
>> disable proxying the object.
>
> Thanks for the explanation.
>
> How thread-safe is this approach? Suppose two threads concurrently
> access the same replacer. Each grabs hold of a reference to the
> replacer in the original scope entry. Then one thread continues,
> replaces the object and cleans it up. The other thread might be
> just at the beginning of __getattribute__ at that moment, perhaps
> past the _real_obj access. So it won't proxy, even if proxying
> were enabled. Instead it will encounter a cleaned replacer, thus
> raising the kind of exception we're talking about here. Did I miss
> something?

It isn't threadsafe. But bzrlib itself has never claimed to be
threadsafe.

It has come up with Loggerhead, so we've tweaked it a bit to be closer
to threadsafe.

>
>> However, when pydoc runs, it touches everything in the module,
>> creating copies, but before they've had a chance to get
>> 'replaced' into the real objects.
>
> I guess the internals of this "creating copies" is the reason why
> importing modules is safe but importing classes is not, right?

I think the big reason is that objects are sometimes useful as is
(like a constant, or in an 'isinstance' check). And there is no way to
trigger the import if you don't access an attribute. (we overload
__call__ and __getattr__, IIRC).

>
>> I'd like to continue fixing these as we see them.
>
> As importing objects instead of modules appears to be quite common
> in python, we should imho do something more about them than just
> fixing them when whe see them.
>
> One option that comes to my mind are doctests whether pydoc will
> work. This requires spawning of a new python process for every
> module to test, adding considerable time to the selftest suite.
> Would probably detect other pydoc-related issues as well.
>
> Another option would be actually disallowing lazy imports of
> anything but modules, instead of just discouraging them. In other
> words, if the ScopeReplacer (actually ImportReplacer, right?) ever
> gets replaced by anything but a module, raise an exception then.
>
> I guess the second approach would be the cleaner solution. Gave it
> a try in lp:~gagern/bzr/lazy_import-force_modules .
>
> -- Martin
>

With care, you don't have to only import modules, certainly its been
working for everything but pydoc. I personally don't use pydoc for
code I'm working on, because the documentation is right there in the
code. I've used it for some things, but I tend to use online docs,
because they are hyperlinked.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5b0BAACgkQJdeBCYSNAAPQOwCeJTLEQSmxsgmuo1xZ1R6u4Eyw
/MgAnRk9jJ4mc6K33Guc6hOm4Ed/sEfj
=dLvk
-----END PGP SIGNATURE-----