[Twisted-Python] Twisted Names strings
After updating Twisted to 12.3.0 I started getting errors like the following one 2013-01-17 14:38:23+0100 [-] File "/usr/lib64/python2.7/site-packages/twisted/names/common.py", line 93, in lookupAddress 2013-01-17 14:38:23+0100 [-] return self._lookup(name, dns.IN, dns.A, timeout) 2013-01-17 14:38:23+0100 [-] File "/usr/lib64/python2.7/site-packages/twisted/names/client.py", line 384, in _lookup 2013-01-17 14:38:23+0100 [-] d = self.queryUDP([dns.Query(name, type, cls)], timeout) 2013-01-17 14:38:23+0100 [-] File "/usr/lib64/python2.7/site-packages/twisted/names/dns.py", line 484, in __init__ 2013-01-17 14:38:23+0100 [-] self.name = Name(name) 2013-01-17 14:38:23+0100 [-] File"/usr/lib64/python2.7/site-packages/twisted/names/dns.py", line 357, in __init__ 2013-01-17 14:38:23+0100 [-] raise TypeError("%r is not a byte string" % (name,)) 2013-01-17 14:38:23+0100 [-] TypeError: u'example.com' is not a byte string Taking a look at documentation I found this document http://twistedmatrix.com/documents/current/core/howto/python3.html which states "twisted.names.dns deals with strings with a wide range of meanings, often several for each DNS record type. Most of these strings have remained as byte strings, which will probably require application updates (for the reason given in the FilePath section above). Some strings have changed to text strings, though. Any string representing a human readable address (for example, Record_A's address parameter) is now a text string. Additionally, time-to-live (ttl) values given as strings must now be given as text strings." which seems to describe the problem I'm experiencing. But let me say that I'm currently using Python 2.7.3 and I was surprised in realizing that a simple update from a minor 12.x release was enough to disrupt my application. Don't you think that a kind of compatibility mechanism (maybe with a warning message) would have been a more gentle way of doing this transition especially for those who are still using Python 2? Thanks and regards, -- Angelo Dell'Aera 'buffer' Antifork Research, Inc. http://buffer.antifork.org Sysenter Honeynet Project http://www.sysenter-honeynet.org
On 04:22 pm, angelo.dellaera@gmail.com wrote:
Absolutely. This issue has been reported in connection to Wokkel already: http://twistedmatrix.com/trac/ticket/6245 There is currently a fix proposed and waiting review, and there has been some discussion about doing a maintenance release of 12.3 to distribute the fix. Thank you for reporting your experience with 12.3.0 (although testing during the /pre/-release period would have saved everyone some trouble :). It would be great if you could try out the proposed fix and let us know if it also addresses the problem with your application. Since some concern has been voiced over the effort necessary to do a 12.3.1 release (as opposed to just including this fix in 13.0), I'd also be interested to know whether one or the other of those would be more helpful for you - does anything prevent you (or your users) from upgrading to 13.0 once it is released? Thanks, Jean-Paul
On Jan 17, 2013, at 9:19 AM, exarkun@twistedmatrix.com wrote:
(although testing during the /pre/-release period would have saved everyone some trouble :).
This bears repeating! Not to harass you, Angelo - dealing with a regression in Twisted that breaks your app is punishment enough :) - but to remind everyone that this is exactly why we have the pre-release testing period. When you get the 13.0 release announcement, please remember that you only have a week! All you need to do is to run your respective test suite and send a short email saying if you found any issues. (Filing a ticket would be nice, but if that's too much work, don't worry about it: for pre-release regressions, we'll file tickets and work on patches for you.) -glyph
Honestly, if you have comprehensive automated testing and plan to upgrade Twisted regularly or to try and support the latest release it is worth the effort to setup testing against Twisted trunk. https://github.com/dreid/treq/blob/master/.travis.yml shows how I do this with travis-ci. -David On Thu, Jan 17, 2013 at 12:15 PM, Glyph <glyph@twistedmatrix.com> wrote:
On Jan 17, 2013, at 12:57 PM, David Reid <dreid@dreid.org> wrote:
Honestly, if you have comprehensive automated testing and plan to upgrade Twisted regularly or to try and support the latest release it is worth the effort to setup testing against Twisted trunk.
https://github.com/dreid/treq/blob/master/.travis.yml shows how I do this with travis-ci.
Thanks for the pointer, David. I will disseminate this more widely. It's also important for the Twisted team to find out about failures though, so it's still important to _look_ at your travis-ci build results in the pre-release week to make sure that everything is still working, and let us know if it isn't. It would be great if we could automate that, too; would it be possible to set up a page that just has at-a-glance build results for 3rd-party projects against twisted trunk? -glyph
David Reid <dreid@dreid.org> wrote:
In this particular case, the problem was an integration issue. All Wokkel's test cases passed (over at travis-ci), but the code setting up clients was calling SRVConnect with a value that is now being rejected. I.e. it used to happily, but erroneously, take unicode strings with ASCII compatible code points. Apart from actually using a client, no unit tests would have caught this, I believe. That said, I fully take blame for not making time to test Wokkel against the pre-release. -- ralphm
On 2013-01-17 23:09, Ralph Meijer wrote:
(interestingly, K-9 only sent my reply in the plain-text version:) In this particular case, the problem was an integration issue. All Wokkel's test cases passed (over at travis-ci), but the code setting up clients was calling SRVConnect with a value that is now being rejected. I.e. it used to happily, but erroneously, take unicode strings with ASCII compatible code points. Apart from actually using a client, no unit tests would have caught this, I believe. That said, I fully take blame for not making time to test Wokkel against the pre-release. -- ralphm
On Thu, 17 Jan 2013 12:15:03 -0800 Glyph <glyph@twistedmatrix.com> wrote:
Hi Glyph, you're absolutely right and I'll try to do the best I can to contribute to such effort from now on. But let me say that I usually take a look at NEWS.txt before upgrading in order to understand if the changes can impact my code and act properly in such case. I think this case is quite different because I see no attempts to properly handle code which worked perfectly fine in 12.2.0 (if the string is not a bytestring raise TypeError and game over) and the new bytestring requirement is not mentioned at all in NEWS.txt. Just thinking that maybe being a little more backward-compatible and/or verbose in ChangeLog when introducing potentially disrupting changes could greatly help in identifying issues like this one. Thanks, -- Angelo Dell'Aera 'buffer' Antifork Research, Inc. http://buffer.antifork.org Sysenter Honeynet Project http://www.sysenter-honeynet.org
On Jan 18, 2013, at 6:23 AM, Angelo Dell'Aera <angelo.dellaera@gmail.com> wrote:
Thanks, I appreciate that :).
The problem is not that we knew it was going to break things and we didn't care - it's that we didn't notice that the backwards compatibility problem had crept in, and we mistakenly believed that existing users would be using byte strings, or that existing users using unicode strings would already have been broken. Or we simply forgot. The reason we need you to test your programs is so that we notice that there is a mistake that needs fixing. If we already knew about the compatibility problem, we would not have written release notes about it, we would have fixed it before the release :). -glyph
On Thu, 17 Jan 2013 17:19:01 -0000 exarkun@twistedmatrix.com wrote:
Sure. I'll test it during this weekend and let you know if it solves the problem.
There's nothing which prevents me from upgrading from 12.x to 13.0 so I think that after testing the proposed patch properly fixes the issue I can just wait for the 13.0 release. Thanks, -- Angelo Dell'Aera 'buffer' Antifork Research, Inc. http://buffer.antifork.org Sysenter Honeynet Project http://www.sysenter-honeynet.org
On Thu, 17 Jan 2013 17:19:01 -0000 exarkun@twistedmatrix.com wrote:
Hi, I didn't have the possibility to test the fix during the last days (hopefully will do in the next hours) but taking a look at the fix proposed by ralphm http://twistedmatrix.com/trac/changeset/36867 it seems like the patch is not complete because a similar check (and potential conversion) should be inserted in the twisted.names.dns.Name __init__ method too. Am I wrong? Ciao, -- Angelo Dell'Aera 'buffer' Antifork Research, Inc. http://buffer.antifork.org Sysenter Honeynet Project http://www.sysenter-honeynet.org
FYI, this change also broke wokkel, too, so at least you're not alone, it seems... https://twistedmatrix.com/trac/ticket/6245 On Thu, Jan 17, 2013 at 5:22 PM, Angelo Dell'Aera <angelo.dellaera@gmail.com
wrote:
-- cheers lvh
On 04:22 pm, angelo.dellaera@gmail.com wrote:
Absolutely. This issue has been reported in connection to Wokkel already: http://twistedmatrix.com/trac/ticket/6245 There is currently a fix proposed and waiting review, and there has been some discussion about doing a maintenance release of 12.3 to distribute the fix. Thank you for reporting your experience with 12.3.0 (although testing during the /pre/-release period would have saved everyone some trouble :). It would be great if you could try out the proposed fix and let us know if it also addresses the problem with your application. Since some concern has been voiced over the effort necessary to do a 12.3.1 release (as opposed to just including this fix in 13.0), I'd also be interested to know whether one or the other of those would be more helpful for you - does anything prevent you (or your users) from upgrading to 13.0 once it is released? Thanks, Jean-Paul
On Jan 17, 2013, at 9:19 AM, exarkun@twistedmatrix.com wrote:
(although testing during the /pre/-release period would have saved everyone some trouble :).
This bears repeating! Not to harass you, Angelo - dealing with a regression in Twisted that breaks your app is punishment enough :) - but to remind everyone that this is exactly why we have the pre-release testing period. When you get the 13.0 release announcement, please remember that you only have a week! All you need to do is to run your respective test suite and send a short email saying if you found any issues. (Filing a ticket would be nice, but if that's too much work, don't worry about it: for pre-release regressions, we'll file tickets and work on patches for you.) -glyph
Honestly, if you have comprehensive automated testing and plan to upgrade Twisted regularly or to try and support the latest release it is worth the effort to setup testing against Twisted trunk. https://github.com/dreid/treq/blob/master/.travis.yml shows how I do this with travis-ci. -David On Thu, Jan 17, 2013 at 12:15 PM, Glyph <glyph@twistedmatrix.com> wrote:
On Jan 17, 2013, at 12:57 PM, David Reid <dreid@dreid.org> wrote:
Honestly, if you have comprehensive automated testing and plan to upgrade Twisted regularly or to try and support the latest release it is worth the effort to setup testing against Twisted trunk.
https://github.com/dreid/treq/blob/master/.travis.yml shows how I do this with travis-ci.
Thanks for the pointer, David. I will disseminate this more widely. It's also important for the Twisted team to find out about failures though, so it's still important to _look_ at your travis-ci build results in the pre-release week to make sure that everything is still working, and let us know if it isn't. It would be great if we could automate that, too; would it be possible to set up a page that just has at-a-glance build results for 3rd-party projects against twisted trunk? -glyph
David Reid <dreid@dreid.org> wrote:
In this particular case, the problem was an integration issue. All Wokkel's test cases passed (over at travis-ci), but the code setting up clients was calling SRVConnect with a value that is now being rejected. I.e. it used to happily, but erroneously, take unicode strings with ASCII compatible code points. Apart from actually using a client, no unit tests would have caught this, I believe. That said, I fully take blame for not making time to test Wokkel against the pre-release. -- ralphm
On 2013-01-17 23:09, Ralph Meijer wrote:
(interestingly, K-9 only sent my reply in the plain-text version:) In this particular case, the problem was an integration issue. All Wokkel's test cases passed (over at travis-ci), but the code setting up clients was calling SRVConnect with a value that is now being rejected. I.e. it used to happily, but erroneously, take unicode strings with ASCII compatible code points. Apart from actually using a client, no unit tests would have caught this, I believe. That said, I fully take blame for not making time to test Wokkel against the pre-release. -- ralphm
On Thu, 17 Jan 2013 12:15:03 -0800 Glyph <glyph@twistedmatrix.com> wrote:
Hi Glyph, you're absolutely right and I'll try to do the best I can to contribute to such effort from now on. But let me say that I usually take a look at NEWS.txt before upgrading in order to understand if the changes can impact my code and act properly in such case. I think this case is quite different because I see no attempts to properly handle code which worked perfectly fine in 12.2.0 (if the string is not a bytestring raise TypeError and game over) and the new bytestring requirement is not mentioned at all in NEWS.txt. Just thinking that maybe being a little more backward-compatible and/or verbose in ChangeLog when introducing potentially disrupting changes could greatly help in identifying issues like this one. Thanks, -- Angelo Dell'Aera 'buffer' Antifork Research, Inc. http://buffer.antifork.org Sysenter Honeynet Project http://www.sysenter-honeynet.org
On Jan 18, 2013, at 6:23 AM, Angelo Dell'Aera <angelo.dellaera@gmail.com> wrote:
Thanks, I appreciate that :).
The problem is not that we knew it was going to break things and we didn't care - it's that we didn't notice that the backwards compatibility problem had crept in, and we mistakenly believed that existing users would be using byte strings, or that existing users using unicode strings would already have been broken. Or we simply forgot. The reason we need you to test your programs is so that we notice that there is a mistake that needs fixing. If we already knew about the compatibility problem, we would not have written release notes about it, we would have fixed it before the release :). -glyph
On Thu, 17 Jan 2013 17:19:01 -0000 exarkun@twistedmatrix.com wrote:
Sure. I'll test it during this weekend and let you know if it solves the problem.
There's nothing which prevents me from upgrading from 12.x to 13.0 so I think that after testing the proposed patch properly fixes the issue I can just wait for the 13.0 release. Thanks, -- Angelo Dell'Aera 'buffer' Antifork Research, Inc. http://buffer.antifork.org Sysenter Honeynet Project http://www.sysenter-honeynet.org
On Thu, 17 Jan 2013 17:19:01 -0000 exarkun@twistedmatrix.com wrote:
Hi, I didn't have the possibility to test the fix during the last days (hopefully will do in the next hours) but taking a look at the fix proposed by ralphm http://twistedmatrix.com/trac/changeset/36867 it seems like the patch is not complete because a similar check (and potential conversion) should be inserted in the twisted.names.dns.Name __init__ method too. Am I wrong? Ciao, -- Angelo Dell'Aera 'buffer' Antifork Research, Inc. http://buffer.antifork.org Sysenter Honeynet Project http://www.sysenter-honeynet.org
FYI, this change also broke wokkel, too, so at least you're not alone, it seems... https://twistedmatrix.com/trac/ticket/6245 On Thu, Jan 17, 2013 at 5:22 PM, Angelo Dell'Aera <angelo.dellaera@gmail.com
wrote:
-- cheers lvh
participants (6)
-
Angelo Dell'Aera -
David Reid -
exarkun＠twistedmatrix.com -
Glyph -
Laurens Van Houtven -
Ralph Meijer