Opened 4 years ago

Last modified 13 days ago

#4935 defect new

spread.jelly doesn't properly resolve some circular references

Reported by: speth Owned by: exarkun
Priority: normal Milestone:
Component: pb Keywords: jelly review
Cc: warner, wolfgang.kde@… 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 (4)

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

Download all attachments as: .zip

Change History (12)

comment:1 Changed 4 years ago by DefaultCC Plugin

  • Cc warner added

Changed 4 years ago by speth

Test case demonstrating the failure

Changed 4 years ago by speth

Patch that fixes the issue

comment:2 Changed 3 weeks ago by wolfgang61

  • Keywords review added

This fix is needed for porting to Python 3

comment:3 Changed 3 weeks 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 3 weeks ago by wolfgang61

comment:4 Changed 3 weeks 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 follow-up: Changed 3 weeks 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.

comment:6 in reply to: ↑ 5 Changed 3 weeks ago by wolfgang61

  • Cc wolfgang.kde@… added
  • Keywords review added
  • Owner changed from wolfgang61 to exarkun

Replying to exarkun:

  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).

I removed those tests. Your proposal would even accept int: type(int) is type. See mailing list.

  1. Please add a news fragment for this feature (as described on ReviewProcess)

Not sure what to do. I have no commit rights. Some text like

"Jelly: fix handling of references for new style classes"

  1. 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.

Not obvious to me either, but I did some research. Added a comment.

Changed 3 weeks ago by wolfgang61

comment:7 Changed 3 weeks ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to wolfgang61

Not sure what to do. I have no commit rights. Some text like

Please add this to the patch along with the rest of the changes. Apart from that, what's not clear?

"Jelly: fix handling of references for new style classes"

Please read the documentation for news fragments on the ReviewProcess page I linked to.

Not obvious to me either, but I did some research. Added a comment.

Thanks for the effort. Unfortunately the comment doesn't help much - particularly since it doesn't actually try to explain what's going on.

comment:8 Changed 13 days ago by wolfgang61

  • Keywords review added
  • Owner changed from wolfgang61 to exarkun

I have an updated patch lying around but am holding it back, waiting to see what happens with Ticket 7628 (new type "port" for NEWS). If you think I should not wait, which NEWS type should this one be?

Note: See TracTickets for help on using tickets.