Skip to content
This repository

close the cursor as soon as possible #27

Closed
wants to merge 1 commit into from

2 participants

Peter Bengtsson Patrick Altman
Peter Bengtsson

The reason for this change is quite complex and something I've only been able to "test" (there are no unit tests) in MySQL but then again MySQL is the only one famous for having this annoying problem (http://dev.mysql.com/doc/refman/5.0/en/gone-away.html)

Basically, what happens is that even though nothing goes wrong I get this on stdout::

    Exception _mysql_exceptions.OperationalError: (2013, 'Lost connection to MySQL server during query')
    in <bound method Cursor.__del__ of <MySQLdb.cursors.Cursor object at 0x1025c1250>> ignored

After a lot of searching in the pdb trenches I think I figured out what it was.

We use the cursor to execute something (e.g. "ALTER TABLE ...") but we don't care about any of the results. Then, nashvegas proceeds with a bunch of other things that might take time (e.g. executing post sync signals) and once all of that is done the objects are torn down and collected and when it does this, the cursor.del method is executed just before the python process shuts down and returns to the OS. Inside cursor.__del__ (see code below) it first exhausts what's been fetched and deletes it but because this happens such a long time since the cursor was executed you get that dreaded error (it's actually trapped as just a warning my MySQLdb).

The relevant code in MySQLdb/cursor.py:

    def __del__(self):
        self.close()
        self.errorhandler = None
        self._result = None

    def close(self):
        """Close the cursor. No further queries will be possible."""
        if not self.connection: return
        while self.nextset(): pass
        self.connection = None

    def nextset(self):
        if self._executed:
            self.fetchall()         
        del self.messages[:]

    db = self._get_db()
        nr = db.next_result()    # this is where MySQLdb barfs the exception 
        if nr == -1:
            return None
        self._do_get_result()
        self._post_get_result()
        self._warning_check()
        return 1

Like I said the exception is swallowed but we've experienced other weird undeterministic exceptions and errors with nashvegas and MySQL and I think it might help to at least use the dbapi properly as that makes things less likely to cause trouble when wrapped in Django's transaction magic clothing.

Patrick Altman
Owner

Just to be clear the only code change here is line 187 where you close the cursor, right?

That's right.

Patrick Altman
Owner

I really appreciate the pull request and the detailed write up and explanation. I don't currently use MySQL anywhere but I do remember seeing this exception from time to time when I used to use MySQL in other environments (still Django but not nashvegas). I'll make some time in the coming week or so to test this out on my existing project (Postgres) just to make sure it doesn't regress and then I'll pull and do a release.

Thanks!

Peter Bengtsson

Thanks. yeah, don't ask me why we're using MySQL when there is Postgres :)

Most of our problems we have with nashvegas + mysql are ultra weird and incredibly hard to reproduce. I think two titans are fighting for the attention of transaction management (django 1 and mysql 1) and somewhere there things go wrong sometimes.

Check out this one for example: http://jhonjairoroa87.blogspot.com/2011/03/mysql-operationalerror-error-on-rename.html

Patrick Altman
Owner

So does this solve #26 as well? They seem like the same error message.

I apologize for the delay in dealing with this but will try my best to get your fix merged, tested, released this week.

Peter Bengtsson

I honestly don't know if it solves #26 because the error we get there is non-deterministic. It's not unlikely it'll fix it because I suspect lingering cursors might be the reason.

Peter Bengtsson

Any progress on looking at the pull request? It's a shame that it's not possible to make a reproduce-able case we can prove with a unit test but I think that it at least is more complete this way.

Patrick Altman
Owner

I merged this manually and tagged/released 0.6.4 (now on pypi)

Patrick Altman paltman closed this October 04, 2011
Peter Bengtsson

Thanks! When time allows I'm going to upgrade us and see if it removes some of the weirdness problems we've had with mysql.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

Aug 19, 2011
Peter Bengtsson close the cursor as soon as possible 22d9c19
This page is out of date. Refresh to see the latest.

Showing 1 changed file with 37 additions and 37 deletions. Show diff stats Hide diff stats

  1. 74  nashvegas/management/commands/upgradedb.py
74  nashvegas/management/commands/upgradedb.py
@@ -27,7 +27,7 @@ class MigrationError(Exception):
27 27
 
28 28
 
29 29
 class Command(BaseCommand):
30  
-    
  30
+
31 31
     option_list = BaseCommand.option_list + (
32 32
         make_option("-l", "--list", action = "store_true",
33 33
                     dest = "do_list", default = False,
@@ -56,26 +56,26 @@ class Command(BaseCommand):
56 56
     help = "Upgrade database."
57 57
 
58 58
     def _filter_down(self, stop_at=None):
59  
-        
  59
+
60 60
         if stop_at is None:
61 61
             stop_at = float("inf")
62  
-        
  62
+
63 63
         applied = []
64 64
         to_execute = []
65 65
         scripts_in_directory = []
66  
-        
  66
+
67 67
         try:
68 68
             already_applied = Migration.objects.all().order_by("migration_label")
69  
-            
  69
+
70 70
             for x in already_applied:
71 71
                 applied.append(x.migration_label)
72  
-            
  72
+
73 73
             in_directory = os.listdir(self.path)
74 74
             in_directory = [migration for migration in in_directory if
75 75
                             not migration.startswith(".")]
76 76
             in_directory.sort()
77 77
             applied.sort()
78  
-            
  78
+
79 79
             for script in in_directory:
80 80
                 name, ext = os.path.splitext(script)
81 81
                 match = MIGRATION_NAME_RE.match(name)
@@ -85,7 +85,7 @@ def _filter_down(self, stop_at=None):
85 85
                 number = int(match.group(1))
86 86
                 if ext in [".sql", ".py"]:
87 87
                     scripts_in_directory.append((number, script))
88  
-            
  88
+
89 89
             for number, script in scripts_in_directory:
90 90
                 if script not in applied and number <= stop_at:
91 91
                     to_execute.append(script)
@@ -99,13 +99,13 @@ def _get_rev(self, fpath):
99 99
         Get an SCM verion number. Try svn and git.
100 100
         """
101 101
         rev = None
102  
-        
  102
+
103 103
         try:
104 104
             cmd = ["git", "log", "-n1", "--pretty=format:\"%h\"", fpath]
105 105
             rev = Popen(cmd, stdout=PIPE, stderr=PIPE).communicate()[0]
106 106
         except:
107 107
             pass
108  
-        
  108
+
109 109
         if not rev:
110 110
             try:
111 111
                 cmd = ["svn", "info", fpath]
@@ -116,9 +116,9 @@ def _get_rev(self, fpath):
116 116
                         rev = tokens[1].strip()
117 117
             except:
118 118
                 pass
119  
-            
  119
+
120 120
         return rev
121  
-    
  121
+
122 122
     def init_nashvegas(self):
123 123
         # Copied from line 35 of django.core.management.commands.syncdb
124 124
         # Import the 'management' module within each installed app, to register
@@ -139,7 +139,7 @@ def init_nashvegas(self):
139 139
                 msg = exc.args[0]
140 140
                 if not msg.startswith("No module named") or "management" not in msg:
141 141
                     raise
142  
-        
  142
+
143 143
         # @@@ make cleaner / check explicitly for model instead of looping over and doing string comparisons
144 144
         connection = connections[self.db]
145 145
         cursor = connection.cursor()
@@ -149,22 +149,22 @@ def init_nashvegas(self):
149 149
                 cursor.execute(s)
150 150
                 transaction.commit_unless_managed(using=self.db)
151 151
                 return
152  
-    
  152
+
153 153
     def create_migrations(self):
154 154
         statements = get_sql_for_new_models(self.args)
155 155
         if len(statements) > 0:
156 156
             for s in statements:
157 157
                 print s
158  
-    
  158
+
159 159
     @transaction.commit_manually
160 160
     def execute_migrations(self, show_traceback=False):
161 161
         migrations = self._filter_down()
162  
-        
  162
+
163 163
         if not len(migrations):
164 164
             sys.stdout.write("There are no migrations to apply.\n")
165  
-        
  165
+
166 166
         created_models = set()
167  
-        
  167
+
168 168
         try:
169 169
             for migration in migrations:
170 170
                 migration_path = os.path.join(self.path, migration)
@@ -172,18 +172,19 @@ def execute_migrations(self, show_traceback=False):
172 172
                 lines = fp.readlines()
173 173
                 fp.close()
174 174
                 content = "".join(lines)
175  
-                
  175
+
176 176
                 if migration_path.endswith(".sql"):
177 177
                     to_execute = "".join(
178 178
                         [l for l in lines if not l.startswith("### New Model: ")]
179 179
                     )
180 180
                     connection = connections[self.db]
181 181
                     cursor = connection.cursor()
182  
-                    
  182
+
183 183
                     sys.stdout.write("Executing %s... " % migration)
184  
-                    
  184
+
185 185
                     try:
186 186
                         cursor.execute(to_execute)
  187
+                        cursor.close()
187 188
                     except Exception:
188 189
                         sys.stdout.write("failed\n")
189 190
                         if show_traceback:
@@ -191,20 +192,20 @@ def execute_migrations(self, show_traceback=False):
191 192
                         raise MigrationError()
192 193
                     else:
193 194
                         sys.stdout.write("success\n")
194  
-                    
  195
+
195 196
                     for l in lines:
196 197
                         if l.startswith("### New Model: "):
197 198
                             created_models.add(
198 199
                                 get_model(
199 200
                                     *l.replace("### New Model: ", "").strip().split(".")
200  
-                                ) 
  201
+                                )
201 202
                             )
202 203
                 elif migration_path.endswith(".py"):
203 204
                     sys.stdout.write("Executing %s... " % migration)
204  
-                    
  205
+
205 206
                     module = {}
206 207
                     execfile(migration_path, {}, module)
207  
-                    
  208
+
208 209
                     if "migrate" in module and callable(module["migrate"]):
209 210
                         try:
210 211
                             module["migrate"]()
@@ -215,7 +216,7 @@ def execute_migrations(self, show_traceback=False):
215 216
                             raise MigrationError()
216 217
                         else:
217 218
                             sys.stdout.write("success\n")
218  
-                
  219
+
219 220
                 Migration.objects.create(
220 221
                     migration_label=migration,
221 222
                     content=content,
@@ -241,7 +242,7 @@ def execute_migrations(self, show_traceback=False):
241 242
             raise
242 243
         else:
243 244
             transaction.commit(using=self.db)
244  
-    
  245
+
245 246
     def seed_migrations(self, stop_at=None):
246 247
         # @@@ the command-line interface needs to be re-thinked
247 248
         try:
@@ -263,18 +264,18 @@ def seed_migrations(self, stop_at=None):
263 264
                 print m.migration_label, "has been seeded"
264 265
             else:
265 266
                 print m.migration_label, "was already applied."
266  
-    
267  
-    
  267
+
  268
+
268 269
     def list_migrations(self):
269 270
         migrations = self._filter_down()
270 271
         if len(migrations) == 0:
271 272
             print "There are no migrations to apply."
272 273
             return
273  
-        
  274
+
274 275
         print "Migrations to Apply:"
275 276
         for script in migrations:
276 277
             print "\t%s" % script
277  
-    
  278
+
278 279
     def handle(self, *args, **options):
279 280
         """
280 281
         Upgrades the database.
@@ -287,23 +288,22 @@ def handle(self, *args, **options):
287 288
         self.do_create = options.get("do_create")
288 289
         self.do_seed = options.get("do_seed")
289 290
         self.args = args
290  
-        
  291
+
291 292
         self.path = options.get("path")
292 293
         self.verbosity = int(options.get("verbosity", 1))
293 294
         self.interactive = options.get("interactive")
294 295
         self.db = options.get("database", DEFAULT_DB_ALIAS)
295  
-        
  296
+
296 297
         self.init_nashvegas()
297 298
 
298 299
         if self.do_create:
299 300
             self.create_migrations()
300  
-        
  301
+
301 302
         if self.do_execute:
302 303
             self.execute_migrations(show_traceback=True)
303  
-        
  304
+
304 305
         if self.do_list:
305 306
             self.list_migrations()
306  
-        
  307
+
307 308
         if self.do_seed:
308 309
             self.seed_migrations()
309  
-
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.