Ticket #21751 review requested

[Michael Manfre]

In an effort to make Django a bit more explicit with regards to closing cursors when they are no longer needed, I have created ticket #21751 [1] with a pull request[2].

Most of the changes are straight forward and change the usage of a cursor so that it uses a context manager or closes the cursor in 'finally'. 

Example:

# old pattern

connection.cursor().execute(...)

# changes to

with connection.cursor() as cursor:

   cursor.execute(...)

The biggest change is with SQLCompiler.execute_sql. I added new constants to make the usage of execute_sql more explicit (and self documenting) for the cases when the caller wants no result and when the caller wants to fetch the results from the cusor directly.

[1] https://code.djangoproject.com/ticket/21751

[2] https://github.com/django/django/pull/2154

Regards,

Michael Manfre

[Shai Berger]

On Friday 17 January 2014 01:19:29 Michael Manfre wrote:
> In an effort to make Django a bit more explicit with regards to closing
> cursors when they are no longer needed, I have created ticket #21751 [1]
> with a pull request[2].
>
> Most of the changes are straight forward and change the usage of a cursor
> so that it uses a context manager or closes the cursor in 'finally'.
>
> Example:
>
> # old pattern
> connection.cursor().execute(...)
>
> # changes to
> with connection.cursor() as cursor:
> cursor.execute(...)
>

I think this is suboptimal, API-wise. Python destroys temporaries soon enough.
Is there a reason why we cannot arrange for a __del__ method to close the
cursor? Circular references anywhere?

Shai.

[Łukasz Rekucki]

On 19 January 2014 09:12, Shai Berger <sh...@platonix.com> wrote:
> On Friday 17 January 2014 01:19:29 Michael Manfre wrote:
>> In an effort to make Django a bit more explicit with regards to closing
>> cursors when they are no longer needed, I have created ticket #21751 [1]
>> with a pull request[2].
>>
>> Most of the changes are straight forward and change the usage of a cursor
>> so that it uses a context manager or closes the cursor in 'finally'.
>>
>> Example:
>>
>> # old pattern
>> connection.cursor().execute(...)
>>
>> # changes to
>> with connection.cursor() as cursor:
>> cursor.execute(...)
>>
>
> I think this is suboptimal, API-wise. Python destroys temporaries soon enough.

You mean CPython, right? Considering that a DB cursor can also
allocate resources in the DB itself, I think it's not a good idea to
rely on GC here. Unfortunately, the pull request in question won't
help in many cases due to the way how try: finally: works in
generators[1].

> Is there a reason why we cannot arrange for a __del__ method to close the
> cursor? Circular references anywhere?

AFAIK, this is already the case as long as the wrapped cursor does it.

[1]: https://bugs.pypy.org/issue736

--
Łukasz Rekucki

[Aymeric Augustin]

On 19 janv. 2014, at 09:12, Shai Berger <sh...@platonix.com> wrote:

> I think this is suboptimal, API-wise. Python destroys temporaries soon enough.
> Is there a reason why we cannot arrange for a __del__ method to close the
> cursor? Circular references anywhere?

Hi Shai,

You probably meant CPython ;-) I believe this change will improve consistency with other Python implementations, notably PyPy.

Following your reasoning, we should also stop bothering about closing files, as Python will do it. But that's a bad practice and Python 3.3+ even raises warnings in that situation. Is there a difference between file descriptors and cursors (in practice TCP sockets) that justifies treating them differently? (Honest question — I don’t know.)

--
Aymeric.

[Shai Berger]

On Sunday 19 January 2014 11:52:40 Łukasz Rekucki wrote:
> On 19 January 2014 09:12, Shai Berger <sh...@platonix.com> wrote:
> > On Friday 17 January 2014 01:19:29 Michael Manfre wrote:
> >> In an effort to make Django a bit more explicit with regards to closing
> >> cursors when they are no longer needed, I have created ticket #21751 [1]
> >> with a pull request[2].
> >>
> >> Most of the changes are straight forward and change the usage of a
> >> cursor so that it uses a context manager or closes the cursor in
> >> 'finally'.
> >>
> >> Example:
> >>
> >> # old pattern
> >> connection.cursor().execute(...)
> >>
> >> # changes to
> >>
> >> with connection.cursor() as cursor:
> >> cursor.execute(...)
> >
> > I think this is suboptimal, API-wise. Python destroys temporaries soon
> > enough.
>
> You mean CPython, right? Considering that a DB cursor can also
> allocate resources in the DB itself, I think it's not a good idea to
> rely on GC here. Unfortunately, the pull request in question won't
> help in many cases due to the way how try: finally: works in
> generators[1].
>

Yep, I had CPython's behavior in mind. Didn't realize how different the others
were in this regard.

Still, spreading with-blocks all over the code for this looks very ugly to me.
I'd rather add an execute_single() or execute_and_close() method to the
cursors instead. Perhaps even only as private API, as that usage is mostly
common within Django's code.

Shai.

[Michael Manfre]

On Sun, Jan 19, 2014 at 5:23 AM, Shai Berger <sh...@platonix.com> wrote:

Still, spreading with-blocks all over the code for this looks very ugly to me.
I'd rather add an execute_single() or execute_and_close() method to the
cursors instead. Perhaps even only as private API, as that usage is mostly
common within Django's code.

You would rather deviate from PEP 249 to avoid requiring the users of a cursor to explicitly close it? Interesting...

Regards,

Michael Manfre

[Shai Berger]

No. I'd rather extend the interface defined by PEP 249 in order to reduce the
amount of RY in the code. Your PR has at least 10 instances of

with connection.cursor() as cursor:
cursor.execute(...)

# block ends here

and I think around 10 more of variations on the theme

with connection.cursor() as cursor:
cursor.execute(...)

do_something_with(cursor.fetchone())

In similar, though less general fashion, I would add to
tests/introspection/tests.py a simple module-level function:

def get_table_description(*args):
with connection.cursor as cursor:
return connection.introspection.get_table_description(cursor, *args)

There are many other instances in the PR where a cursor defined at the with
statement is used for more than one database command; I see those as net
improvement.

(I must admit that my initial reaction was "colored" by your other PR[1],
where these one-line-with-blocks seem to be a larger percentage of the with-
blocks. Still).

Shai.

[1] https://github.com/django/django/pull/2143

[Michael Manfre]

I agree with Aymeric in that cursors are very much like files. Wrapping a single line of code in a with (or a try-finally) is a standard pattern and allows Django to follow PEP 249 without extra modifications.

The overall feedback I've gotten from my pull request seems positive. I'll rebase it against master and submit it for merging.

Regards,

Michael Manfre

--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/2404579.hcV3zBsRXv%40deblack.

For more options, visit https://groups.google.com/groups/opt_out.

[Shai Berger]

Just to make things clear, my reservations about the use of 'with' syntax with
cursors do not amount to an objection to this PR. I have not reviewed it again
since the rebase, but I did like most of what I've seen before. The with-
blocks can be cleaned up later, if I or anyone else find it justified; the hooks
for 3rd-party backends should probably land as soon as possible.

Shai.

[Aymeric Augustin]

Hi Michael,

On 21 janv. 2014, at 19:05, Michael Manfre <mma...@gmail.com> wrote:
> The overall feedback I've gotten from my pull request seems positive. I'll rebase it against master and submit it for merging.

Let me know when you do this. I'll push the merge button before the patch gets out of date :-)

Thank you,

--
Aymeric.

[Michael Manfre]

The pull request has been rebased.

Thanks,

Michael Manfre

--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/20E8A9D6-74E6-4913-9B5B-F0710BC4F745%40polytechnique.org.