Code

Opened 2 years ago

Closed 6 months ago

#17956 closed Cleanup/optimization (duplicate)

Explicit closing cursors is required for the consistency in multithread environment

Reported by: nnseva Owned by: nobody
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords: thread, threading, cursor, close, finalization, __del__, out of sync, twisted
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Using django ORM in multithread environment causes rare (ignored) exceptions like 'ProgrammingError: (2014, "Commands out of sync; you can't run this command now")' in Cursor.__del__ method (this particular error happens when using MySQL backend).

I've met this problem using django ORM (over MySQL) together with twisted.thread.deferToThread call.

The problem happens relatively rare, but is critical for 24/7 services, because looks like causing regular memory leaks.

Deep investigating the code concerned to the exception contexts, I've found that almost no one cursor got by django ORM from the backend is closed explicitly.

I've changed the django code to implement explicit cursor closing in all found contexts. After these changes my service leaved for 12 hours of continuous working shown no one exception like this.

All changes have been made in one file, django/db/models/sql/compiler.py

Changes have been made in the following functions and methods:

  • SQLCompiler.execute_sql in SINGLE and MULTI cases
  • SQLInsertCompiler.execute_sql
  • SQLUpdateCompiler.execute_sql
  • order_modified_iter
  • also not_modified_iter iterator (like order_modified_iter) has been added to catch iterator finalization stage

All changes are explicitly closing the cursor got from the backend after use.

The applied patch has been made for django v.1.3.1.final

Attachments (1)

explicit_cursor_closing.patch (3.0 KB) - added by nnseva 2 years ago.
Patch for django 1.3.1.final to close opened cursors explicitly

Download all attachments as: .zip

Change History (3)

Changed 2 years ago by nnseva

Patch for django 1.3.1.final to close opened cursors explicitly

comment:1 Changed 2 years ago by akaariai

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

The reasoning in the description seems valid. I guess the error is the garbage collector tries to collect the cursor while another thread is doing some work. Seems like some backend libraries do not like that. I haven't actually tested this, but in any case closing cursors explicitly is recommended, and we should do it.

It would be great if you could provide a self-contained test case. Of course, having a test in Django's test suite would be perfect, but seems hard to achieve.

comment:2 Changed 6 months ago by timo

  • Resolution set to duplicate
  • Status changed from new to closed

I think this is a duplicate of #21751 which has a patch to close cursors after they are used.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.