[Django] #19895: Second iteration over an invalid queryset returns an empty list instead of an exception

Django

unread,

Feb 23, 2013, 12:51:42 PM2/23/13

to django-...@googlegroups.com

#19895: Second iteration over an invalid queryset returns an empty list instead of
an exception
----------------------------------------------+--------------------
Reporter: gnosek | Owner: gnosek
Type: Uncategorized | Status: new
Component: Database layer (models, ORM) | Version: 1.4
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
As a part of #17664 it was discovered that an invalid queryset only raises
exceptions during the first iteration. When iterating over the queryset
again, an empty list is returned, i.e. the following test case would fail:

{{{#!python
def test_invalid_qs_list(self):
qs = Article.objects.order_by('invalid_column')
self.assertRaises(FieldError, list, qs)
self.assertRaises(FieldError, list, qs)
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/19895>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

Django

unread,

Feb 23, 2013, 12:51:53 PM2/23/13

to django-...@googlegroups.com

#19895: Second iteration over an invalid queryset returns an empty list instead of
an exception

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by gnosek):

* status: new => assigned
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/19895#comment:1>

Django

unread,

Feb 23, 2013, 1:40:17 PM2/23/13

to django-...@googlegroups.com

#19895: Second iteration over an invalid queryset returns an empty list instead of
an exception

-------------------------------------+-------------------------------------
Reporter: gnosek | Owner: gnosek

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------

Comment (by gnosek):

All the solutions I can come up with are apparently ugly. I'm attaching
two versions of the patch for discussion (with tests stripped).

One solution is wrapping the iterator in another method, the other is
putting the required try/catch in the iterator() method itself, which
pushes the indentation to six levels deep maximum.

--
Ticket URL: <https://code.djangoproject.com/ticket/19895#comment:2>

Django

unread,

Feb 23, 2013, 1:42:08 PM2/23/13

to django-...@googlegroups.com

#19895: Second iteration over an invalid queryset returns an empty list instead of
an exception

-------------------------------------+-------------------------------------
Reporter: gnosek | Owner: gnosek

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by gnosek):

* needs_better_patch: 0 => 1
* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/19895#comment:3>

Django

unread,

Feb 23, 2013, 3:03:20 PM2/23/13

to django-...@googlegroups.com

#19895: Second iteration over an invalid queryset returns an empty list instead of
an exception

-------------------------------------+-------------------------------------
Reporter: gnosek | Owner: gnosek

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by gnosek):

* needs_better_patch: 1 => 0

Comment:

As suggested by jacobkm on IRC, here's the updated patch:

https://github.com/django/django/pull/800

--
Ticket URL: <https://code.djangoproject.com/ticket/19895#comment:4>

Django

unread,

Feb 23, 2013, 3:39:58 PM2/23/13

to django-...@googlegroups.com

#19895: Second iteration over an invalid queryset returns an empty list instead of
an exception

-------------------------------------+-------------------------------------
Reporter: gnosek | Owner: gnosek
Type: Uncategorized | Status: closed

Component: Database layer | Version: 1.4

(models, ORM) | Resolution: fixed

Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Jacob Kaplan-Moss <jacob@…>):

* status: assigned => closed
* resolution: => fixed

Comment:

In [changeset:"d1e87eb3baf75b1b6a0ada46a9b77f7e347cdb60"]:
{{{
#!CommitTicketReference repository=""
revision="d1e87eb3baf75b1b6a0ada46a9b77f7e347cdb60"
[1.5.x] Fixed #19895 -- Made second iteration over invalid queryset raise
an exception too

When iteration over a queryset raised an exception, the result cache
remained initialized with an empty list, so subsequent iterations returned
an empty list instead of raising an exception

Backport of 2cd0edaa477b327024e4007c8eaf46646dcd0f21 from master.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/19895#comment:5>

Django

unread,

Mar 11, 2013, 8:43:34 AM3/11/13

to django-...@googlegroups.com

#19895: Second iteration over an invalid queryset returns an empty list instead of
an exception

-------------------------------------+-------------------------------------
Reporter: gnosek | Owner: gnosek
Type: Bug | Status: new

Component: Database layer | Version: 1.4
(models, ORM) | Resolution:

Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by claudep):

* status: closed => new
* severity: Normal => Release blocker

* needs_better_patch: 0 => 1

* type: Uncategorized => Bug
* resolution: fixed =>
* stage: Unreviewed => Accepted

Comment:

That commit is causing a serious ORM memory leak in one of my
applications. It may be that my code is not the cleanest, but anyway, I
consider this as a serious regression.

--
Ticket URL: <https://code.djangoproject.com/ticket/19895#comment:6>

Django

unread,

Mar 18, 2013, 1:52:16 PM3/18/13

to django-...@googlegroups.com

#19895: Second iteration over an invalid queryset returns an empty list instead of
an exception

-------------------------------------+-------------------------------------
Reporter: gnosek | Owner: gnosek

Comment (by akaariai):

Attached is a minimalistic test case that will show the memory leak. The
case is simple - have enough objects that one ITERATOR_CHUNK_SIZE will not
convert all the objects (that is, more than 100 objects in the queryset).
Do bool(qs). This will result in memory leak when this ticket's patch is
applied, but will not leak if this ticket's patch isn't applied.

The reason for the leak is a bug in Python itself. The
[http://docs.python.org/2/library/gc.html#gc.garbage gc.garbage] docs say
that:
"""
A list of objects which the collector found to be unreachable but could
not be freed (uncollectable objects). By default, this list contains only
objects with `__del__()` methods. Objects that have `__del__()` methods
and are part of a reference cycle cause the entire reference cycle to be
uncollectable, including objects not necessarily in the cycle but
reachable only from it. ...
"""

However, no `__del__` method is defined anywhere, so there should not be
any uncollectable objects. Also, pypy collects the garbage, so this is
another thing pointing to a bug in Python.

I have tested this with Python 2.7.3 and Python 3.2.3, and both of those
will leak. Pypy 1.8.0 collects the garbage correctly.

Steps to reproduce: unpack the attachment, run tester.py, see if
gc.garbage has reference to _safe_iterator.

Even if this is a bug in Python this has to be fixed in Django itself. The
memory leak can be bad. It seems just reverting the commit is the right
fix.

Interestingly enough doing this change in Query.iterator() is enough to
cause leak:
{{{
try:
iterator() code here...
except Exception:
raise
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/19895#comment:7>

Django

unread,

Mar 18, 2013, 3:45:26 PM3/18/13

to django-...@googlegroups.com

#19895: Second iteration over an invalid queryset returns an empty list instead of
an exception

-------------------------------------+-------------------------------------
Reporter: gnosek | Owner: gnosek

Comment (by akaariai):

Here is a minimalistic case showing the bug in Python:
{{{
class MyObj(object):
def __iter__(self):
self._iter = iter(self.iterator())
return iter(self._iter)

def iterator(self):
try:
while True:
yield 1
except Exception:
raise
i = next(iter(MyObj()))
import gc
gc.collect()
print(gc.garbage)
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/19895#comment:8>

Django

unread,

Mar 18, 2013, 4:41:42 PM3/18/13

to django-...@googlegroups.com

#19895: Second iteration over an invalid queryset returns an empty list instead of
an exception

-------------------------------------+-------------------------------------
Reporter: gnosek | Owner: gnosek

Comment (by akaariai):

I filed a bug to Python bug tracker. http://bugs.python.org/issue17468

Does anybody see any other solution than reverting the patch?

--
Ticket URL: <https://code.djangoproject.com/ticket/19895#comment:9>

Django

unread,

Mar 19, 2013, 2:14:55 AM3/19/13

to django-...@googlegroups.com

#19895: Second iteration over an invalid queryset returns an empty list instead of
an exception

-------------------------------------+-------------------------------------
Reporter: gnosek | Owner: gnosek

Comment (by carljm):

I think we should roll back the patch. Your queryset-iteration
simplification patch will fix this bug anyway, correct?

--
Ticket URL: <https://code.djangoproject.com/ticket/19895#comment:10>

Django

unread,

Mar 19, 2013, 4:32:35 PM3/19/13

to django-...@googlegroups.com

#19895: Second iteration over an invalid queryset returns an empty list instead of
an exception

-------------------------------------+-------------------------------------
Reporter: gnosek | Owner: gnosek

Comment (by akaariai):

The more complex version of the simplification patch has this same issue.
It is likely possible to work around this issue in the patch.

As for 1.5 a roll back seems like the only option.

--
Ticket URL: <https://code.djangoproject.com/ticket/19895#comment:11>

Django

unread,

Mar 20, 2013, 5:43:38 AM3/20/13

to django-...@googlegroups.com

#19895: Second iteration over an invalid queryset returns an empty list instead of
an exception

-------------------------------------+-------------------------------------
Reporter: gnosek | Owner: gnosek
Type: Bug | Status: closed

Component: Database layer | Version: 1.4

(models, ORM) | Resolution: fixed

Changes (by Claude Paroz <claude@…>):

* status: new => closed
* resolution: => fixed

Comment:

In [changeset:"b91067d9aa42e31d4375e00a703beaacdb30d608"]:
{{{
#!CommitTicketReference repository=""
revision="b91067d9aa42e31d4375e00a703beaacdb30d608"
[1.5.x] Revert "Fixed #19895 -- Made second iteration over invalid

queryset raise an exception too"

This reverts commit d1e87eb3baf75b1b6a0ada46a9b77f7e347cdb60.
This commit was the cause of a memory leak. See ticket for more details.
Thanks Anssi Kääriäinen for identifying the source of the bug.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/19895#comment:12>

Django

unread,

Mar 20, 2013, 5:45:46 AM3/20/13

to django-...@googlegroups.com

#19895: Second iteration over an invalid queryset returns an empty list instead of
an exception

-------------------------------------+-------------------------------------
Reporter: gnosek | Owner: gnosek
Type: Bug | Status: new

Component: Database layer | Version: 1.4
(models, ORM) | Resolution:

Changes (by claudep):

* status: closed => new

* resolution: fixed =>

--
Ticket URL: <https://code.djangoproject.com/ticket/19895#comment:13>

Django

unread,

Mar 21, 2013, 9:53:59 PM3/21/13

to django-...@googlegroups.com

#19895: Second iteration over an invalid queryset returns an empty list instead of
an exception

-------------------------------------+-------------------------------------
Reporter: gnosek | Owner: gnosek

Changes (by rcoup):

* cc: robert.coup@… (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/19895#comment:14>

Django

unread,

May 10, 2013, 7:59:45 PM5/10/13

to django-...@googlegroups.com

#19895: Second iteration over an invalid queryset returns an empty list instead of
an exception

-------------------------------------+-------------------------------------
Reporter: gnosek | Owner: gnosek

Type: Bug | Status: new
Component: Database layer | Version: 1.4
(models, ORM) | Resolution:

Severity: Normal | Triage Stage: Accepted

Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Changes (by akaariai):

* severity: Release blocker => Normal

Comment:

This isn't a release blocker any more, the leak is fixed, the second
iteration works in the same way as before.

--
Ticket URL: <https://code.djangoproject.com/ticket/19895#comment:15>

Django

unread,

May 22, 2013, 7:47:25 AM5/22/13

to django-...@googlegroups.com

#19895: Second iteration over an invalid queryset returns an empty list instead of
an exception

-------------------------------------+-------------------------------------
Reporter: gnosek | Owner: gnosek
Type: Bug | Status: closed

Component: Database layer | Version: 1.4

(models, ORM) | Resolution: fixed

Changes (by claudep):

* status: new => closed
* resolution: => fixed

Comment:

Test committed in 904084611d740e26eb3cb44af9a3d2f3a6d1b665

--
Ticket URL: <https://code.djangoproject.com/ticket/19895#comment:16>

Reply all

Reply to author

Forward