Opened 7 years ago

Closed 6 years ago

Last modified 3 years ago

#8115 closed (fixed)

Infinite loop trying to delete circularly referent models

Reported by: kcarnold Owned by: nobody
Component: Database layer (models, ORM) Version: master
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: UI/UX:

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 kcarnold 7 years ago.

Download all attachments as: .zip

Change History (8)

Changed 7 years ago by kcarnold

comment:1 Changed 7 years ago by jacob

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

comment:2 Changed 7 years ago by anonymous

  • Has patch set

comment:3 Changed 7 years ago by jacob

  • Keywords 1.0-blocker added

comment:4 Changed 6 years ago by russellm

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 Changed 6 years ago by jacob

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 Changed 6 years ago by jacob

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

(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 Changed 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

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