Opened 3 years ago

Last modified 19 hours ago

#4935 defect new

spread.jelly doesn't properly resolve some circular references

Reported by: speth Owned by: wolfgang61
Priority: normal Milestone:
Component: pb Keywords: jelly
Cc: warner Branch:
Author: speth Launchpad Bug:

Description

Circular references which are attributes of a new-style class instance are not unjellied properly due to the way the instance dictionary is handled. The resulting unjellied object contains instances of twisted.persisted.crefutil._Dereference objects.

Attached are a set of test cases demonstrating the failure and a patch which corrects them.

Attachments (3)

test_jelly.py (2.1 KB) - added by speth 3 years ago.
Test case demonstrating the failure
jelly.patch (481 bytes) - added by speth 3 years ago.
Patch that fixes the issue
jelly2.patch (4.7 KB) - added by wolfgang61 22 hours ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 3 years ago by DefaultCC Plugin

  • Cc warner added

Changed 3 years ago by speth

Test case demonstrating the failure

Changed 3 years ago by speth

Patch that fixes the issue

comment:2 Changed 24 hours ago by wolfgang61

  • Keywords review added

This fix is needed for porting to Python 3

comment:3 Changed 23 hours ago by multani

The tests really need to be updated:

  • They have to be runnable with the other tests with Trial (ie. use TestCase, use assert-like methods, etc.).
  • They are far from being clear to me; I'm not an expert in jelly, but without any docstrings, un-explicit test names, un-explicit classes names (Foo, Qux?), it's hard to exactly tell what's being tested here.
  • At last, they should not rely on monkey-patching twisted.spread.jelly to understand what's happening.

Changed 22 hours ago by wolfgang61

comment:4 Changed 21 hours ago by wolfgang61

jelly2.patch patches two bugs with new style classes and adds tests triggering those bugs, including all tests from the above test_jelly.py, extended with docstrings.

Some tests from test_jelly.py might meanwhile already have been present in the jelly tests but it can do no harm to add them all.

The new test test_methodSelfIdentityNewStyle triggers another bug in _Unjellier._unjelly_method but since this is about the same problem, I am including this here too. New style classes are of type type and not types.ClassType.

comment:5 Changed 19 hours ago by exarkun

  • Keywords review removed
  • Owner set to wolfgang61

Thanks.

  1. Please fix the docstring formatting. Twisted formatting is """\nwords\n""" (you can surely find examples of this anywhere in Twisted).
  2. Please remove the use of hasattr. hasattr swallows exceptions and makes debugging hard. Use getattr(foo, bar, sentinel) is not sentinel (if the attribute cannot legitimately be None then you can use None as the sentinel).
    1. Why check for __new__ here at all, though? Isn't the intent here to see if the type of the type object is type? The more obvious check seems like it would be type(im_class) is type (leaving aside the question of why the existing code in jelly doesn't use isinstance - which it probably should).
  3. Please add a docstring to test_methodSelfIdentityOldStyle
  4. Please add a docstring to test_methodSelfIdentityNewStyle
  5. Please rewrite the docstrings that say "Check" or "correctly". A test method docstring should explain the desired behavior. It should not say that it tests for "correct" behavior, it should say what that behavior is.
  6. Please also remove the ticket references. This isn't the convention in Twisted.
  7. Please use assertIs instead of assertIdentical (this is a recent change to the coding standard)
  8. Please use {} instead of dict()
  9. Please add a news fragment for this feature (as described on ReviewProcess)
  10. I still don't find it obvious how the change to _newInstance fixes this behavior. Please add a comment - or if you think it's obvious, at least explain it on this ticket.

Thanks again.

Note: See TracTickets for help on using tickets.