Ticket #6166 enhancement new

Opened 8 months ago

Last modified 2 weeks ago

Deprecate pickling support (__getstate__ and __setstate__) in twisted.internet._sslverfiy

Reported by: itamar Owned by:
Priority: low Milestone:
Component: core Keywords: easy gsoc review
Cc: thijs Branch: branches/sslverify-no-pickle-6166-2
(diff, github, buildbot, log)
Author: canibanoglu Launchpad Bug:

Description

Pickleability is no longer a goal for Twisted. As such, the various __getstate__ and __setstate__ methods of twisted.internet._sslverify classes should be deprecated, so we can remove the extra code involved.

Attachments

ticket6166.patch Download (12.1 KB) - added by canibanoglu 2 months ago.
Deprecation of getstate and setstate along with new test which check for deprecation
ticket#6166_2.patch Download (9.3 KB) - added by canibanoglu 2 months ago.
New patch on trunk which deprecates getstate and setstate in twisted.internet._sslverify and adds a 6166.removal in twisted/topfiles
ticket6166_2.patch Download (9.3 KB) - added by canibanoglu 2 months ago.
Removed # from the filename
ticket6166-3.patch Download (5.4 KB) - added by canibanoglu 2 months ago.
Tests now use callDeprecated to test deprecations, use .suppress attribute to suppress deprecation warning messages

Change History

1

Changed 5 months ago by thijs

  • cc thijs added

2

Changed 2 months ago by itamar

  • keywords easy added

See twisted/python/reflect.py and twisted/test/test_reflect.py for an example of deprecation and testing of deprecation.

Changed 2 months ago by canibanoglu

Deprecation of getstate and setstate along with new test which check for deprecation

3

Changed 2 months ago by canibanoglu

  • keywords review added

4

Changed 2 months ago by tom.prince

The patch you attached is reversed, and appears to be against the most recent release, rather than trunk. If you are planning on contributing to twisted, you should be working in a source checkout (see [TwistedDevelopment]).

5

Changed 2 months ago by tom.prince

  • owner set to canibanoglu
  • keywords review removed

The attached patch appears to not apply to trunk.

  1. The deprecations want to use twisted.python.deprecate.deprecated, rather than calling warnings.warn directly. The message should probably say something about pickling not being supported anymore.
  2. Running
    python -Wall bin/trial twisted.test.test_sslverify
    
    gives me a bunch of deprecation warnings. They should be suppressed.

6

Changed 2 months ago by canibanoglu

Thanks a lot for the input.

First of all, the release and trunk version mixup was bordering on idiocy, sorry about that.

1. I have tried using twisted.python.deprecate.deprecated but it doesn't pass the tests because it doesn't work the same way for class methods and module-level functions. I found a ticket which is now marked as fixed but deprecated still acts in a peculiar way. More specifically this is what I get

{{{twisted.trial.unittest.FailTest: not equal: a = 'twisted.internet._sslverify.OpenSSLCertificateOptions.getstate was deprecated in Twisted 13.1.0' b = 'twisted.internet._sslverify.getstate was deprecated in Twisted 13.1.0'}}}

2. Thank you very much for pointing that out, I will definitely take care of that.

Changed 2 months ago by canibanoglu

New patch on trunk which deprecates getstate and setstate in twisted.internet._sslverify and adds a 6166.removal in twisted/topfiles

7

Changed 2 months ago by canibanoglu

  • owner canibanoglu deleted
  • keywords review added

Here's my second go at it.

- I have suppressed deprecation warnings when running the tests with

python -Wall bin/trial twisted.test.test_sslverify

- Still can't get @deprecated to work correctly with class methods. This patch still uses warnings.warn for deprecation warnings. Added "Pickling is no longer supported." to the deprecation warning message.

- Added 6166.removal topfile.

I have a question though. While working on this I have found that @deprecated decorator is not functioning as expected when decorating class methods and the full test run hangs on OS X Mountain Lion (10.8). There are tickets for both of these and they both appear as fixed. Should I create another ticket?

Thanks a lot for the assistance.

8

Changed 2 months ago by exarkun

New patch on trunk which ...

Can you re-attach the patch, this time without a space in the filename? trac cannot serve attachments with spaces in them.

9

Changed 2 months ago by canibanoglu

There's an underscore between 6166 and 2 :) Should I rename them differently from now on?

10

Changed 2 months ago by exarkun

Sorry. I guess there's also a problem with filenames with "#" in them.

Changed 2 months ago by canibanoglu

Removed # from the filename

11

Changed 2 months ago by canibanoglu

Uploaded the same patch file without the "#" character in the filename.

12

Changed 2 months ago by tom.prince

  • keywords gsoc added; review removed
  • owner set to canibanoglu
  1. There are heplers to suppres warnings. See twisted.trial.util.suppress and the .suppress attribute of TestCase and/or test_* methods.
  2. Hmm. Looking at the r24931 diff for #3254, deprecated works with methods, but it needs to called as
     Class.method = deprecated(version)(Class.method)
    
    after the class definition. You should file a ticket to support the obvious thing.
    • (Making this work is probably tricky, and would involve making deprecate return a descriptor)
  3. Is there any reason you created your own helper, rather than using callDeprecated?
  4. The newsfile could be improved by listing all the things that are getting deprecated, rather than saying each individual thing is deprecated, but might be better to just say that pickling the two relevant classes is now deprecated.
  5. Even if not using @deprecated, you should still use the same format for the docstring.

Changed 2 months ago by canibanoglu

Tests now use callDeprecated to test deprecations, use .suppress attribute to suppress deprecation warning messages

13

Changed 2 months ago by canibanoglu

  • owner canibanoglu deleted
  • keywords review added

14

Changed 3 weeks ago by tomprince

  • branch set to branches/sslverify-no-pickle-6166
  • branch_author set to tomprince

(In [38505]) Branching to sslverify-no-pickle-6166.

15

Changed 3 weeks ago by tom.prince

  • status changed from new to assigned
  • owner set to tom.prince

This looks good. A couple of points:

  1. The suppression for test_certificateOptionsSerialization is to general.
  2. CertificateOptions and KeyPair both live publicly in `twisted.inte

rnet.ssl`, so the news should reflect that.

16

Changed 3 weeks ago by tomprince

(In [38507]) Address review comments.

Refs #6166.

17

Changed 3 weeks ago by tomprince

(In [38518]) Be even more specific about what to suppress.

Refs #6166.

18

Changed 3 weeks ago by tom.prince

  • keywords review removed

To expand one (1), we only want to suppress the specific warnings that we are triggering by testing this code. That way, if somebody later changes something else to cause a different warning to occur here, it would still be reported.

Other than those two points, this looks good.

I'll go ahead and merge after fixing those two issues.

19

Changed 3 weeks ago by tomprince

  • status changed from assigned to closed
  • resolution set to fixed

(In [38519]) Merge sslverify-no-pickle-6166: Deprecate pickling support in twisted.internet._sslverfiy.

Authore: canibanoglu Reviewers: tom.prince Fixes: #6166

Pickleability is no longer a goal for Twisted. As such, the various getstate and setstate methods of twisted.internet._sslverify classes can be deprecated, so we can remove the extra code involved.

20

Changed 3 weeks ago by exarkun

  • status changed from closed to reopened
  • resolution fixed deleted

(In [38527]) Revert r38517 - test suite regression

Reopens: #6166

A number of test modules now fail to import on some builders:

[ERROR]
Traceback (most recent call last):
  File "/data/home/buildbot/work/freebsd-8.2-i386/Twisted/twisted/trial/runner.py", line 500, in loadPackage
    module = modinfo.load()
  File "/data/home/buildbot/work/freebsd-8.2-i386/Twisted/twisted/python/modules.py", line 383, in load
    return self.pathEntry.pythonPath.moduleLoader(self.name)
  File "/data/home/buildbot/work/freebsd-8.2-i386/Twisted/twisted/python/_reflectpy3.py", line 266, in namedAny
    topLevelPackage = _importAndCheckStack(trialname)
  File "/data/home/buildbot/work/freebsd-8.2-i386/Twisted/twisted/python/_reflectpy3.py", line 205, in _importAndCheckStack
    return __import__(importName)
  File "/data/home/buildbot/work/freebsd-8.2-i386/Twisted/twisted/test/test_sslverify.py", line 805, in <module>
    class KeyPair(unittest.TestCase):
  File "/data/home/buildbot/work/freebsd-8.2-i386/Twisted/twisted/test/test_sslverify.py", line 808, in KeyPair
    CN=b"server")[0]
  File "/data/home/buildbot/work/freebsd-8.2-i386/Twisted/twisted/test/test_sslverify.py", line 84, in makeCertificate
    keypair = PKey()
exceptions.NameError: global name 'PKey' is not defined

twisted.test.test_sslverify

21

Changed 3 weeks ago by exarkun

(In [38528]) whitespace fixes

refs #6166

22

Changed 3 weeks ago by tomprince

  • branch changed from branches/sslverify-no-pickle-6166 to branches/sslverify-no-pickle-6166-2

(In [38537]) Branching to sslverify-no-pickle-6166-2.

23

Changed 3 weeks ago by tom.prince

  • keywords review added
  • branch_author changed from tomprince to canibanoglu

I think that should address the test failures.

24

Changed 3 weeks ago by tom.prince

  • status changed from reopened to new
  • owner tom.prince deleted

25

Changed 2 weeks ago by glyph

Minor nitpick:  there is a new twisted checker error; a new line is too long.

Note: See TracTickets for help on using tickets.