Closed GoogleCodeExporter closed 9 years ago
Possibly issue 1409 is also relevant -- the problem there is caused by
app-engine-
patch which attempts to delete the imported django modules. This is not safe.
Original comment by guido@google.com
on 4 Jun 2010 at 7:07
Sorry, that would be
http://code.google.com/p/googleappengine/issues/detail?id=1409 --
I forgot this is a different tracker. (And the relevance is likely that
attempts to
delete django from sys.modules are always unsafe.)
Original comment by guido@google.com
on 4 Jun 2010 at 7:17
Thanks for pointing 1409 out, Guido.
For the record, I'd rather see this patch use getattr instead of hasattr to
check the value like in this changeset:
http://code.google.com/p/dherbst-app-engine-django/source/detail?
r=cad9d77a103622c77d894a6b7b52ee2b2ff70fa7 if it is committed so there is no
ambiguity of whether the
imports have successfully completed.
Original comment by dherbst
on 4 Jun 2010 at 7:27
dbherbst: yep, I like your patch. Is there anyone who can review and apply
this?
Original comment by kumar.mcmillan
on 4 Jun 2010 at 11:17
I can review and apply, please ensure you've signed the CLA as described in the
readme file.
Would this patch and issue still be relevant if the helpers import logic for
Django was changed to only support use_library ?
If we can address the underlying cause of the failures by switching to
use_library, rather than simply papering over the symptoms then that would be
preferable.
If you can demonstrate that we still need this logic even in the presence of
use_library then I'm happy to accept the patch, although I'd prefer if all the
logic to determine whether or not the helper has already been initialised would
be contained within the InstallAppengineHelperForDjango method - with task
queues and cron jobs main.py is often not the only entry point into the
application, so the helper is invoked from multiple places. We don't want to
have to duplicate this logic in all those files.
Original comment by m...@google.com
on 8 Jun 2010 at 11:36
Issue 155 has been merged into this issue.
Original comment by m...@google.com
on 8 Jun 2010 at 11:39
I signed the CLA in 2009. I haven't seen this issue with use_library and I do
not use a django zip file so I can't comment. Perhaps Kumar can comment on
this.
Original comment by dherbst
on 8 Jun 2010 at 1:26
The attached zip file, gae-django-reproduce-double-bug.zip, is a complete app
that reproduces the bug when you upload it to an App Engine server. When I get
some free time I'll try applying use_library to that code but if someone beats
me to it then they should be able to prove the theory and get an answer sooner.
I don't know how use_libary works but if it does not involve deleting django
from sys.modules then it should solve the issue just fine. Thanks for taking a
look m...@google!
Original comment by kumar.mcmillan
on 8 Jun 2010 at 6:47
Hi all. Sorry for the delayed response. I'm proposing a new patch, attached
here and explained below.
The first thing I did was fire up the example app that was attached to this
ticket to reproduce the error. The example app still reproduced the error but
note it only exists on App Engine itself, not when running the app from the
local SDK.
In response to m...@google.com's comment about converting to use_django() I am
now confused by this. I see that use_django() is already the method employed
by the code, as you can see in the LoadDjango() method of
appengine_django/__init__.py
So instead this new patch takes the approach that m...@google.com suggested
which is to memoize InstallAppengineHelperForDjango() itself (not main.py) so
it only runs once per process. This will apply the fix for other entry points
(cron, task queue, etc).
The patch fixes the repeated exception that can be seen in the example app and
I also verified that all tests pass. Can someone review this and perhaps get
it applied to trunk? I have already signed the CLA.
thanks!
-Kumar
Original comment by kumar.mcmillan
on 30 Jul 2010 at 6:40
Attachments:
Does any maintainer have a minute to review the latest patch? Thanks, Kumar
Original comment by kumar.mcmillan
on 21 Aug 2010 at 9:15
Hi. Once again, could someone take a look and perhaps apply this patch? It
might help some people out. I read this article about someone who ran into
this and gave up, switching to Rackspace http://www.agmweb.ca/blog/andy/2286/
i think my fix here should solve these odd deadline error states.
Original comment by kumar.mcmillan
on 25 Oct 2010 at 4:18
+1 to kumar's request. I've applied the patch he provided in comment 9 and it
completely solved a whole class of errors that had dumbfounded me in a large,
commercial AppEngine app. Including this patch in the official release would be
a huge help, especially because it's very hard from the error messages to infer
that this error is related to google-app-engine-django.
Original comment by shimonr...@gmail.com
on 25 Oct 2010 at 4:39
This issue was closed by revision r107.
Original comment by m...@google.com
on 26 Oct 2010 at 12:29
Apologies for the delay merging the patch. Thanks very much.
Original comment by m...@google.com
on 26 Oct 2010 at 12:30
thanks m...!
Original comment by kumar.mcmillan
on 26 Oct 2010 at 4:51
Original issue reported on code.google.com by
kumar.mcmillan
on 17 May 2010 at 3:01Attachments: