Opened 2 years ago

Closed 6 months ago

#8110 defect closed duplicate (duplicate)

setting `t.i.b.DelayedCall.debug = True` causes all tests to fail in Python 3

Reported by: posita Owned by: Pawel
Priority: normal Milestone: Python-3.x
Component: core Keywords: py3k
Cc: Branch:
Author:

Description

Consider:

# test_delayedcall_py3.py
from twisted.trial import unittest as t_unittest

class Py3DelayedCallTestCase(t_unittest.TestCase): pass

if __name__ == '__main__':
    import os
    from twisted.internet import base as t_base
    t_base.DelayedCall.debug = bool(os.environ.get('DELAYED_CALL_DEBUG'))
    from unittest import main
    main()

As expected, with or without DelayedCall.debug set to True, it runs in Python 2:

% python2.7 ./test_delayedcall_py3.py
.
----------------------------------------------------------------------
Ran 1 test in 0.017s

OK
% DELAYED_CALL_DEBUG=1 python2.7 ./test_delayedcall_py3.py
.
----------------------------------------------------------------------
Ran 1 test in 0.017s

OK

But not so with Python 3:

% python3.4 ./test_delayedcall_py3.py
.
----------------------------------------------------------------------
Ran 1 test in 0.017s

OK
% DELAYED_CALL_DEBUG=1 python3.4 ./test_delayedcall_py3.py
E
======================================================================
ERROR: runTest (__main__.Py3DelayedCallTestCase)
runTest
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/matt/Documents/dev/txretry/.tox/py34-twisted_trunk/lib/python3.4/site-packages/twisted/internet/defer.py", line 588, in _runCallbacks
    current.result = callback(current.result, *args, **kw)
  File "/Users/matt/Documents/dev/txretry/.tox/py34-twisted_trunk/lib/python3.4/site-packages/twisted/trial/_asynctest.py", line 114, in <lambda>
    d.addBoth(lambda x : call.active() and call.cancel() or x)
  File "/Users/matt/Documents/dev/txretry/.tox/py34-twisted_trunk/lib/python3.4/site-packages/twisted/internet/base.py", line 94, in cancel
    self._str = bytes(self)
TypeError: 'DelayedCall' object is not iterable

----------------------------------------------------------------------
Ran 1 test in 0.022s

FAILED (errors=1)

Attachments (2)

8110_a.diff (3.3 KB) - added by Pawel 20 months ago.
8110.patch (4.7 KB) - added by Pawel 18 months ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 2 years ago by posita

Keywords: py3k added; py3 removed

comment:2 Changed 20 months ago by Pawel

See also #8307 and #8306

Changed 20 months ago by Pawel

Attachment: 8110_a.diff added

comment:3 Changed 20 months ago by Pawel

Keywords: review added

Ok so here's what happened. Debug mode on DelayedCall was not tested. #5987 replaced one call to str() to call to bytes()( see line 94 here: https://twistedmatrix.com/trac/changeset/44579/trunk/twisted/internet/base.py), but in case of DelayedCall object there was str but there was no bytes. This only mattered in one untested case when canceling DelayedCall in debug mode.

Aside from this debug mode is enabled at class level, but important property that matters for this mode (DelayedCall "creator" property) is only called at instance initialization. It means that users setting debug mode on instance (not on class) e.g. here #8307 were getting errors because init was executed with incorrect value of "debug".

So there were 3 bugs here:

  • debug mode was not tested
  • there was no way to set debug mode on instance
  • there was problem in debug mode on cancel()

All this is now fixed see patch attached.

Note that this patch also fixes: #8307 and #8306 so please close those related ticket if patch works ok.

Last edited 20 months ago by Pawel (previous) (diff)

comment:4 Changed 20 months ago by Glyph

Keywords: review removed
Owner: set to Pawel

pawelmhm,

Thanks for your work on this, it's great to have nice debuggability / reliability fixes like this. I have just a little feedback:

  1. Would you mind fixing up the patch to test using the public interface, i.e. str(), rather than the irrelevant internal _str attribute? It seems like the old, bad behavior could be provoked using the public API as well.
  2. Can you add a docstring to setUp?
  3. This decoding should be conditional, since __str__ really should return bytes on py2. Perhaps you could add a __unicode__?

Please address these issues to your satisfaction and re-submit.

Thank you!

comment:5 Changed 18 months ago by Pawel

Keywords: review added
Owner: Pawel deleted

Would you mind fixing up the patch to test using the public interface, i.e. str(), rather than the irrelevant internal _str attribute? It seems like the old, bad behavior could be provoked using the public API as well.

done

Can you add a docstring to setUp?

done

This decoding should be conditional, since str really should return bytes on py2.

After thinking about this I realized bytes() were only used internally in DelayedCall and they were only used in this deprecated _str attribute. After removing _str and updating logic around it there is no longer need for bytes.

Perhaps you could add a unicode?

looking into what str does I dont see any point where it can actually be useful to handle unicode. Maybe I'm missing something but all values handled by str (function name, time, etc) are guaranteed to be ascii. So I wonder if we really need unicode here? Let me know when we need it.

New patch attached.

Changed 18 months ago by Pawel

Attachment: 8110.patch added

comment:6 Changed 18 months ago by Craig Rodrigues

Keywords: review removed
Owner: set to Pawel

There have been a lot of Python 3 fixes committed to trunk, so your patch is outdated. Also, in the past month, the Twisted project has switched to allowing patch contributions via GitHub.

Can you follow the steps in this document, and re-submit your patch via GitHub:

https://twistedmatrix.com/trac/wiki/TwistedDevelopment#SubmittingaPatch

comment:7 Changed 18 months ago by Pawel

Branch: review
Owner: Pawel deleted

There have been a lot of Python 3 fixes committed to trunk, so your patch is outdated.

which part is outdated?

Also, in the past month, the Twisted project has switched to allowing patch contributions via GitHub.Can you follow the steps in this document, and re-submit your patch via GitHub

sure will do that

comment:8 Changed 18 months ago by Pawel

Branch: review
Keywords: review added

comment:9 Changed 18 months ago by Craig Rodrigues

Keywords: review removed
Owner: set to Pawel

I think your patch might be OK, but let's confirm after you submit as per: https://twistedmatrix.com/trac/wiki/TwistedDevelopment#SubmittingaPatch

and some of the automated tests run. Please set back to review after you have done so.

comment:10 Changed 6 months ago by Craig Rodrigues

Resolution: duplicate
Status: newclosed

Duplicate of ticket:8995

Note: See TracTickets for help on using tickets.