Opened 4 years ago

Last modified 5 weeks 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
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 (5)

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 4 months ago.
jelly3.patch (5.7 KB) - added by wolfgang61 4 months ago.
jelly4.patch (6.5 KB) - added by wolfgang61 3 months ago.

Download all attachments as: .zip

Change History (19)

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 4 months ago by wolfgang61

  • Keywords review added

This fix is needed for porting to Python 3

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

comment:4 Changed 4 months 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 4 months 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 4 months 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 4 months ago by wolfgang61

comment:7 Changed 4 months 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 3 months 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?

Changed 3 months ago by wolfgang61

comment:9 Changed 3 months ago by wolfgang61

New patch attached with a full explanation why the change in _newInstance() is needed.

comment:10 Changed 2 months ago by exarkun

(In [43287]) apply jelly4.patch

refs #4935

comment:11 Changed 2 months ago by exarkun

(In [43323]) Remove (accidentally included, I think) Python 3 porting related change.

There is no ClassType on Python 3 but this ticket will just
work on fixing a bug in jelly support of the type type.

refs #4935

comment:12 Changed 2 months ago by exarkun

(In [43324]) Adjust the comment explaining why this version of the code is correct

The previous comment was clear enough for me to finally understand what the bug
here is and why this change fixes it. Hopefully this comment is clear enough that
other people might be able to understand it too.

refs #4935

comment:13 Changed 2 months ago by exarkun

(In [43325]) Move this warning out of the dict unjellier method and into the class docstring

The warning applies to more than just dictionaries.
Also expand it to discuss the consequences of this issue.

refs #4935

comment:14 Changed 5 weeks ago by exarkun

  • Keywords review removed

Thanks. The most recent patch makes a lot more sense to me. There are still some testing improvements that I think could be made. I'll try to make them.

Note: See TracTickets for help on using tickets.