Opened 7 years ago

Closed 7 years ago

Last modified 5 years ago

#13366 closed (fixed)

Model importing race condition created in [12977] for subclasses of abstract classes that have fields.

Reported by: Brett Hoerner Owned by: nobody
Component: Core (Other) Version: master
Severity: Keywords:
Cc: adulic@…, safariman@…, davisd.davisd@…, rokclimb15@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

We ran into a weird edge case race condition that seems to be caused by [12977]. FWIW I haven't taken all the time needed to understand exactly why or what [12977] does, I'm just reporting the error based on a traceback and example code set. :)

Basic things you need:

  1. An abstract base class with a field, and a model A that uses it, so that ModelBase.__new__ will drop into the "new_class.add_to_class(field.name, copy.deepcopy(field))" branch intended for abstract only.
  2. Another model (in a different module) that imports model A.
  3. A management command that imports model A (this won't happen in the shell, I'm not sure what the difference is yet or where else it would occur).

Basically it seems that the new "self.default = self.model._meta.get_field_by_name(self.name)[0].default" results eventually in a "get_models()", but this is run *while model A is being loaded* which means that all models are fetched, including the models file which contains model B and thus imports model A, and then blammo, circular dependency problem.

I've attached a simple project with two apps and a sample management command.

Attachments (2)

13366-example.tar.gz (3.0 KB) - added by Brett Hoerner 7 years ago.
Simple example project, run python manage.py fail
13366.symptomaticpatch (1.6 KB) - added by Erik Stein 7 years ago.
Attached symptomatic patch for using the comments app (worksforme).

Download all attachments as: .zip

Change History (13)

Changed 7 years ago by Brett Hoerner

Attachment: 13366-example.tar.gz added

Simple example project, run python manage.py fail

comment:1 Changed 7 years ago by Ramiro Morales

#13368 and a post in django-dev (http://groups.google.com/group/django-developers/browse_frm/thread/be2085f11e9f95e?hl=en) report a manifestation of this when trying to import django.contrib.comments.models.Comment from an app models module.

comment:2 Changed 7 years ago by Russell Keith-Magee

milestone: 1.2
Triage Stage: UnreviewedAccepted

comment:3 Changed 7 years ago by Russell Keith-Magee

Component: UncategorizedCore framework

comment:4 Changed 7 years ago by Russell Keith-Magee

I think the solution here is to force the population of the application cache before management commands are used. The simple approach of putting "get_apps()" early in the management command execute cycle fixes this particular problem, but it causes other regressions in the test suite (mostly dealing with error handling of applications that can't be imported, etc).

comment:5 Changed 7 years ago by anonymous

Cc: adulic@… added

comment:6 Changed 7 years ago by anonymous

Cc: safariman@… added

comment:7 Changed 7 years ago by Patryk Zawadzki

It would be better to guard the self.model access with a hasattr check. copy() should be safe to call at *all* times. Postponing the call is a workaround, not a fix.

Changed 7 years ago by Erik Stein

Attachment: 13366.symptomaticpatch added

Attached symptomatic patch for using the comments app (worksforme).

comment:8 Changed 7 years ago by David Davis

Cc: davisd.davisd@… added

comment:9 Changed 7 years ago by rokclimb15

Cc: rokclimb15@… added

comment:10 Changed 7 years ago by Russell Keith-Magee

Resolution: fixed
Status: newclosed

(In [13005]) Fixed #13366 -- Corrected the field setstate method to avoid a race condition when initially importing models. Thanks to Brett Hoerner for the report.

comment:11 Changed 5 years ago by Jacob

milestone: 1.2

Milestone 1.2 deleted

Note: See TracTickets for help on using tickets.
Back to Top