Opened 16 years ago

Closed 16 years ago

Last modified 12 years ago

#8115 closed (fixed)

Infinite loop trying to delete circularly referent models

Reported by: Kenneth Arnold Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Keywords: 1.0-blocker
Cc: kenneth.arnold@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

ConceptNet has Assertions, RawAssertions, and Ratings (among other models). The relationship looks like:

RawAssertion:
  assertion = ForeignKey(Assertion, null=True)
Rating:
  assertion = ForeginKey(Assertion)
Assertion:
  canonical = ForeignKey(RawAssertion)

Deleting an Assertion runs into an infinite loop in CollectedObjects.ordered_keys. The second time through the loop, when dealing with model=Rating, it has no children, but we shouldn't be setting found=True, because there's still a cyclic dependency. We should be able to tell that because it's already in dealt_with.

I tried to come up with a test case for this, but I couldn't make it fail. You can certainly reproduce it by downloading ConceptNet yourself.

Attached patch fixes the obvious problem by causing it to raise the CyclicDependency exception properly. By "fix", I mean it gets to the real problem: there's a cycle. Eventually we'll need to look at the null flags on the relations to figure out where to break the cycle, but that's a separate issue.

Attachments (1)

django-cyclic-dependency-8115.patch (553 bytes ) - added by Kenneth Arnold 16 years ago.

Download all attachments as: .zip

Change History (8)

by Kenneth Arnold, 16 years ago

comment:1 by Jacob, 16 years ago

milestone: 1.0
Triage Stage: UnreviewedAccepted

comment:2 by anonymous, 16 years ago

Has patch: set

comment:3 by Jacob, 16 years ago

Keywords: 1.0-blocker added

comment:4 by Russell Keith-Magee, 16 years ago

I can't work out a test case for this. At this point, even an extremely complex example drawn from ConceptNet that fails would be most helpful.

comment:5 by Jacob, 16 years ago

I can't reproduce this either. However, I think I'm going to hold my nose and check it in for 1.0 -- the only harm it could cause is to raise more CyclicDependency than strictly necessary. That's better than infinite loops, at least, and it's easier to clean up in the future when we figure out the actual fix.

comment:6 by Jacob, 16 years ago

Resolution: fixed
Status: newclosed

(In [8807]) Fixed #8115: avoid a infiniate loop when collecting related objects for deletion.

I can't reproduce the original error leading to #8115 and the patch. However, the only harm this change could cause is to raise more CyclicDependency exceptions than strictly necessary. That's better than infinite loops, at least, and it's easier to clean up in the future when we figure out the actual fix.

comment:7 by Jacob, 12 years ago

milestone: 1.0

Milestone 1.0 deleted

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