Opened 10 years ago
Closed 8 years ago
#6166 enhancement closed fixed (fixed)
Deprecate pickling support (__getstate__ and __setstate__) in twisted.internet._sslverfiy
Reported by: | Itamar Turner-Trauring | Owned by: | Tom Prince |
---|---|---|---|
Priority: | low | Milestone: | |
Component: | core | Keywords: | easy gsoc |
Cc: | Thijs Triemstra | Branch: |
branches/sslverify-no-pickle-6166-4
branch-diff, diff-cov, branch-cov, buildbot |
Author: | canibanoglu, tomprince |
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 (4)
Change History (40)
comment:1 Changed 9 years ago by
Cc: | Thijs Triemstra added |
---|
comment:2 Changed 9 years ago by
Keywords: | easy added |
---|
Changed 9 years ago by
Attachment: | ticket6166.patch added |
---|
Deprecation of getstate and setstate along with new test which check for deprecation
comment:3 Changed 9 years ago by
Keywords: | review added |
---|
comment:4 Changed 9 years ago by
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]).
comment:5 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Can Ibanoglu |
The attached patch appears to not apply to trunk.
- The deprecations want to use
twisted.python.deprecate.deprecated
, rather than callingwarnings.warn
directly. The message should probably say something about pickling not being supported anymore. - Running
python -Wall bin/trial twisted.test.test_sslverify
gives me a bunch of deprecation warnings. They should be suppressed.
comment:6 Changed 9 years ago by
Thanks a lot for the input.
First of all, the release and trunk version mixup was bordering on idiocy, sorry about that.
- 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'}}}
- Thank you very much for pointing that out, I will definitely take care of that.
Changed 9 years ago by
Attachment: | ticket#6166_2.patch added |
---|
New patch on trunk which deprecates getstate and setstate in twisted.internet._sslverify and adds a 6166.removal in twisted/topfiles
comment:7 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Can Ibanoglu deleted |
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.
comment:8 Changed 9 years ago by
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.
comment:9 Changed 9 years ago by
There's an underscore between 6166 and 2 :) Should I rename them differently from now on?
comment:10 Changed 9 years ago by
Sorry. I guess there's also a problem with filenames with "#" in them.
comment:11 Changed 9 years ago by
Uploaded the same patch file without the "#" character in the filename.
comment:12 Changed 9 years ago by
Keywords: | gsoc added; review removed |
---|---|
Owner: | set to Can Ibanoglu |
- There are heplers to suppres warnings. See
twisted.trial.util.suppress
and the.suppress
attribute ofTestCase
and/ortest_*
methods. - Hmm. Looking at the r24931 diff for #3254,
deprecated
works with methods, but it needs to called asClass.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)
- (Making this work is probably tricky, and would involve making
- Is there any reason you created your own helper, rather than using
callDeprecated
? - 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.
- Even if not using
@deprecated
, you should still use the same format for the docstring.
Changed 9 years ago by
Attachment: | ticket6166-3.patch added |
---|
Tests now use callDeprecated to test deprecations, use .suppress attribute to suppress deprecation warning messages
comment:13 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Can Ibanoglu deleted |
comment:14 Changed 9 years ago by
Author: | → tomprince |
---|---|
Branch: | → branches/sslverify-no-pickle-6166 |
(In [38505]) Branching to sslverify-no-pickle-6166.
comment:15 Changed 9 years ago by
Owner: | set to Tom Prince |
---|---|
Status: | new → assigned |
This looks good. A couple of points:
- The suppression for
test_certificateOptionsSerialization
is to general. CertificateOptions
andKeyPair
both live publicly in `twisted.inte
rnet.ssl`, so the news should reflect that.
comment:17 Changed 9 years ago by
comment:18 Changed 9 years ago by
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.
comment:19 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
(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.
comment:20 Changed 9 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
(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
comment:22 Changed 9 years ago by
Branch: | branches/sslverify-no-pickle-6166 → branches/sslverify-no-pickle-6166-2 |
---|
(In [38537]) Branching to sslverify-no-pickle-6166-2.
comment:23 Changed 9 years ago by
Author: | tomprince → canibanoglu |
---|---|
Keywords: | review added |
I think that should address the test failures.
comment:24 Changed 9 years ago by
Owner: | Tom Prince deleted |
---|---|
Status: | reopened → new |
comment:25 Changed 9 years ago by
Minor nitpick: there is a new twisted checker error; a new line is too long.
comment:28 Changed 9 years ago by
Owner: | set to habnabit |
---|
comment:29 Changed 9 years ago by
Owner: | changed from habnabit to Richard Wall |
---|---|
Status: | new → assigned |
comment:30 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Richard Wall to Tom Prince |
Status: | assigned → new |
Notes:
- Merges cleanly
Points:
- Build Failures
- Python3 tests are very unhappy: https://buildbot.twistedmatrix.com/builders/python-3.3-tests/builds/1405/steps/shell/logs/stdio
- The circularChainWarning failure is something that has since been fixed in trunk I think (r39074)
- The output of python3 tests seems unusual. It only mentions failed and skipped tests, not the ones that passed.
- It also says it dropped to a debugger which I don't remember seeing before.
- So try merging forward? Maybe that'll fix all the skips too.
- Deprecation version numbers need updating. 13.1.0 > 13.2.0
- source:branches/sslverify-no-pickle-6166-2/twisted/test/test_sslverify.py
- "util.suppress(category = DeprecationWarning" -- remove whitespace in the argument assignment.
- "Test deprecation" -- remove the "Test" from test docstrings.
- "test_setstateDeprecation.suppress" --
- Do you have to pass a full state dictionary in the setstat test?
- If you could just pass an empty dict (or minimal dummy dict) this extra suppression might not be necessary.
- The wrapping and message formatting looks strange -- don't the quoted strings need to be surrounded in round brackets?
- Fix the whitespace in the argument assignment.
- "KeyPair" many of the same comments apply to this new testcase.
- source:branches/sslverify-no-pickle-6166-2/twisted/internet/_sslverify.py
- Can you move the deprecation decorators directly beneath the affected function, to make it obvious that it's deprecated.
Please, merge forward, address or answer the numbered points above and link to clean build results then merge.
Thanks.
-RichardW.
comment:31 Changed 8 years ago by
Author: | canibanoglu → canibanoglu, tomprince |
---|---|
Branch: | branches/sslverify-no-pickle-6166-2 → branches/sslverify-no-pickle-6166-3 |
(In [42225]) Branching to sslverify-no-pickle-6166-3.
comment:32 Changed 8 years ago by
The decorators have to be after the class definition, unfortunately.
comment:33 Changed 8 years ago by
Keywords: | review added |
---|---|
Owner: | Tom Prince deleted |
comment:34 Changed 8 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Tom Prince |
Thanks.
- There's a twistedchecker error in
_sslverify.py
that it might be nice to fix here. - The suppression is defined as a regular expression. This code takes advantage of that fact in some ways but ignores it in others - for example, it doesn't escape the "." module separator.
- It's unfortunate that 14.0 hasn't been released yet. I think that 14.1 is the correct version for this deprecation. It wouldn't surprise me if some people try to restart the 14.0 release process based on a new trunk revision though, that would cause a problem here. I don't know what to do about that except blame those people for screwing things up if that happens, though.
Please address those points and then merge. Thanks.
comment:35 Changed 8 years ago by
Branch: | branches/sslverify-no-pickle-6166-3 → branches/sslverify-no-pickle-6166-4 |
---|
(In [42375]) Branching to sslverify-no-pickle-6166-4.
comment:36 Changed 8 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
(In [42380]) Merge sslverify-no-pickle-6166-4: Deprecate pickling support in twisted.internet._sslverfiy.
Author: canibanoglu, tom.prince Reviewer: exarkun, tom.prince, rwall Fixes: #6166
Pickling twisted.internet.ssl.OptionSSLCertificationOptions and twisted.internet.ssl.Keypair is no longer supported.
See
twisted/python/reflect.py
andtwisted/test/test_reflect.py
for an example of deprecation and testing of deprecation.