Create separate classes for each SOCKS error

[felipedau]

These changes intend to make the improvement proposed in #214.

I made a very simple factory, inspired by the strings dictionary. What do you think?

I'd also like to know your opinion on the class names (specially SucceededReply).

I tried not to change things too much, so _disconnect now expects error_message to be either a message or a code (would the name of the arg have to change?). If the code is found, it will instantiate the proper error. Otherwise it will be a SocksError with the message. That implies that if error_message is a code, there must exist a corresponding error because then the SocksError message will be just the code. Should it always be cast to str?

Do you think the factory should always create an error or just when it can actually create the specific classes? I think it should fail to create when the code is unknown, as it is currently.

If that seems a bit strange, maybe _disconnect should expect a SocksError instead of a message and the object should be created before the calls by each method.

Let me know if you would like that I followed some pattern/convention/etc.

Thanks!

[coveralls]

Coverage decreased (-0.08%) to 99.804% when pulling ea9278e934d6759fcfe7d24b0d4ef0f0e41018d4 on felipedau:socks-errors into 0d65a792c97bd2dcb118a939e709d03dd6fad596 on meejah:master.

[coveralls]

Coverage decreased (-0.08%) to 99.804% when pulling ea9278e934d6759fcfe7d24b0d4ef0f0e41018d4 on felipedau:socks-errors into 0d65a792c97bd2dcb118a939e709d03dd6fad596 on meejah:master.

[coveralls]

Coverage decreased (-0.08%) to 99.804% when pulling ea9278e934d6759fcfe7d24b0d4ef0f0e41018d4 on felipedau:socks-errors into 0d65a792c97bd2dcb118a939e709d03dd6fad596 on meejah:master.

[coveralls]

Coverage decreased (-0.08%) to 99.804% when pulling ea9278e934d6759fcfe7d24b0d4ef0f0e41018d4 on felipedau:socks-errors into 0d65a792c97bd2dcb118a939e709d03dd6fad596 on meejah:master.

[coveralls]

Coverage decreased (-0.08%) to 99.804% when pulling ea9278e934d6759fcfe7d24b0d4ef0f0e41018d4 on felipedau:socks-errors into 0d65a792c97bd2dcb118a939e709d03dd6fad596 on meejah:master.

[codecov-io]

Codecov Report

Merging #216 into master will increase coverage by <.01%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #216      +/-   ##
==========================================
+ Coverage   99.88%   99.88%   +<.01%     
==========================================
  Files          20       20              
  Lines        3535     3566      +31     
==========================================
+ Hits         3531     3562      +31     
  Misses          4        4

Impacted Files	Coverage Δ
txtorcon/socks.py	100% <100%> (ø)	:white_check_mark:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0d65a79...939e135. Read the comment docs.

[coveralls]

Coverage decreased (-0.08%) to 99.804% when pulling 6f99e5b85dc5fda2cb4140f29839b1353090f9ea on felipedau:socks-errors into 0d65a792c97bd2dcb118a939e709d03dd6fad596 on meejah:master.

[felipedau]

Looks good so far!

Thanks!

I wouldn't include SuccessReply as an error, and instead it should be an error to ask the error-factory to make an error for code 0x00.

So should that be just a separate class without any relationship? Would it make sense / is it worth it making a SocksReply superclass inherited by both SuccessReply and SocksError?

Also, do you think we could use SuccessReply.code instead of SUCCEEDED?

I think you're right about _on_disconnect: it should receive the SocksError subclass, not the error-message. If that turns out to be a pain for some reason it's fine to leave it, too (as far as this PR).

I modified the signatures, it seems to work!

I also had to modify a test, which makes me think we should probably work on tests too.

Thank you very much @meejah, the code looks a lot better with you suggestions!

[decentral1se]

:beers:

[meejah]

@felipedau hmm, I see what you mean; SucceededReply is because you need something to pass to _on_disconnect?

The rest of the changes look great, thanks!

[meejah]

It just feels a little weird to have a "succeeded error", but I guess actually Twisted does this too :/

Hmm. I wonder if it would work to pass "None" to _on_disconnect if it's a success (and just special-case a check for 0x00?). If that's too weird too, the "success error" is fine -- it's like ConnectionDone in Twisted then.

Also it looks like Python3 builders are sad; it seems Exception doesn't have a .message by default? Or something?

[meejah]

Looking closer, I don't think there's a code-path where SucceededReply would ever get instantiated (because we check for success before calling the error-factory). At least, none that the unit-tests hit ;) nor the web-client examples.

So I think it can just be deleted.

...or maybe it needs more unit-tests to cover this case, e.g. line 291 self._on_disconnect(error.message) but looking at the state-machine I think I'm convinced that the only paths to the _disconnect output are via "actual error paths"; success will go to _make_connection or _domain_name_resolved.

p.s. "make diagrams" will make a GraphVIz diagram of the statemachine (thanks to Automat).

[felipedau]

Hmm. I wonder if it would work to pass "None" to _on_disconnect if it's a success (and just special-case a check for 0x00?). If that's too weird too, the "success error" is fine -- it's like ConnectionDone in Twisted then. [...] @felipedau hmm, I see what you mean; SucceededReply is because you need something to pass to _on_disconnect? [...] Looking closer, I don't think there's a code-path where SucceededReply would ever get instantiated (because we check for success before calling the error-factory). At least, none that the unit-tests hit ;) nor the web-client examples.

So I think it can just be deleted.

...or maybe it needs more unit-tests to cover this case, e.g. line 291 self._on_disconnect(error.message) but looking at the state-machine I think I'm convinced that the only paths to the _disconnect output are via "actual error paths"; success will go to _make_connection or _domain_name_resolved.

Well I didn't think that it would be passed because the factory is only called when reply != self.SUCCEEDED but I thought that it could be useful if some day it needs to be used, and as all these things are replies, maybe they should have some kind of relationship? (Would there be a reason to disconnect on success?)

I also think that can be deleted, unless you see a reason to use it in the future. What I am thinking is that either _SocksMachine.SUCCEEDED or SucceededReply should exist, but not both.

One thing I forgot to mention - there are these two assertions in test/test_socks.py:

152         self.assertIn('Unknown SOCKS reply code', str(ctx.exception)) 
[...]
598         self.assertTrue('Unknown SOCKS reply code' in str(ctx.exception)) 

Those are only passing because I set the factory's default message to "Unknown SOCKS reply code {}", differently from what you suggested. From their scope, it looks like they are really exceptions/errors. So maybe those tests (and the factory's default message) should be updated to error-code?

Also it looks like Python3 builders are sad; it seems Exception doesn't have a .message by default? Or something?

That's confusing:

Python 3.4.2 (default, Oct  8 2014, 10:45:20) 
[GCC 4.9.1] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from pprint import pprint
>>> e = Exception('Message')
>>> str(e)
'Message'
>>> pprint({a: getattr(e, a) for a in dir(e)})
{'__cause__': None,
 '__class__': <class 'Exception'>,
 '__context__': None,
 '__delattr__': <method-wrapper '__delattr__' of Exception object at 0x7fb1abe1b570>,
 '__dict__': {},
 '__dir__': <built-in method __dir__ of Exception object at 0x7fb1abe1b570>,
 '__doc__': 'Common base class for all non-exit exceptions.',
 '__eq__': <method-wrapper '__eq__' of Exception object at 0x7fb1abe1b570>,
 '__format__': <built-in method __format__ of Exception object at 0x7fb1abe1b570>,
 '__ge__': <method-wrapper '__ge__' of Exception object at 0x7fb1abe1b570>,
 '__getattribute__': <method-wrapper '__getattribute__' of Exception object at 0x7fb1abe1b570>,
 '__gt__': <method-wrapper '__gt__' of Exception object at 0x7fb1abe1b570>,
 '__hash__': <method-wrapper '__hash__' of Exception object at 0x7fb1abe1b570>,
 '__init__': <method-wrapper '__init__' of Exception object at 0x7fb1abe1b570>,
 '__le__': <method-wrapper '__le__' of Exception object at 0x7fb1abe1b570>,
 '__lt__': <method-wrapper '__lt__' of Exception object at 0x7fb1abe1b570>,
 '__ne__': <method-wrapper '__ne__' of Exception object at 0x7fb1abe1b570>,
 '__new__': <built-in method __new__ of type object at 0x9c8580>,
 '__reduce__': <built-in method __reduce__ of Exception object at 0x7fb1abe1b570>,
 '__reduce_ex__': <built-in method __reduce_ex__ of Exception object at 0x7fb1abe1b570>,
 '__repr__': <method-wrapper '__repr__' of Exception object at 0x7fb1abe1b570>,
 '__setattr__': <method-wrapper '__setattr__' of Exception object at 0x7fb1abe1b570>,
 '__setstate__': <built-in method __setstate__ of Exception object at 0x7fb1abe1b570>,
 '__sizeof__': <built-in method __sizeof__ of Exception object at 0x7fb1abe1b570>,
 '__str__': <method-wrapper '__str__' of Exception object at 0x7fb1abe1b570>,
 '__subclasshook__': <built-in method __subclasshook__ of type object at 0x9c8580>,
 '__suppress_context__': False,
 '__traceback__': None,
 'args': ('Message',),
 'with_traceback': <built-in method with_traceback of Exception object at 0x7fb1abe1b570>}
>>> 

I just searched for message in https://docs.python.org/3.6/library/exceptions.html but did not find anything that seemed important. Can we make a message @property? Maybe now they want us to use str(e) to access the messages?

p.s. "make diagrams" will make a GraphVIz diagram of the statemachine (thanks to Automat).

Omg that's amazing! Thanks :D

Also, one thing that did not make me very comfortable was when I read the "custom" errors changes:

150                     self.version_error(SocksError(                                                  
  1                         "Expected version 5, got {}".format(version)))

I am not sure if I've got issues (and that's probably the reason of the discussion above), but there are times which I suddenly notice my code has been hit by the "Laser Beam of Java" and, e. g., where I really wanted to make a call with just a simple string now requires an object in order to work :O

P.S. I am going to write a simple test to make sure the instances are properly created according to the code.

Thanks @meejah!

[coveralls]

Coverage decreased (-0.08%) to 99.804% when pulling ca21a1c122bb3da9709811b1e3071bd992147b87 on felipedau:socks-errors into 0d65a792c97bd2dcb118a939e709d03dd6fad596 on meejah:master.

[meejah]

Python3: I think if you just "self.message = message or self.message" you'll fix it. This probably shadows the Exception one in Py2, but oh well. And yes, the "recommended" thing is to always str(e) if you wanted the string-representation. I think.

Tests: yes feel free to change the error-strings in the test asserts. They could even assert on the correct error-class now (e.g. assertTrue(isinstance(ctx.exception, TheSpecificSocksError))) if you want to be "more correct" :) (It is kind of bad to depend on the str() of the Exception, but also I'm pragmatic ;)

For SucceedReply lets just delete it. If there's a use-case later it can be easily added. So keep SUCCEEDED I guess that means.

For the "custom error messages": I'm not sure what you mean? If you think these should be Exception (or similar?) instead of SocksError that's fine too. Or maybe you mean they want their own class too; either way is fine with me I think. They're really "shouldn't happen, but there's weird SOCKS servers out there" errors so I really doubt the calling code can do anything besides tell the user "it didn't work, because 'some technobable'".

[meejah]

Also thanks for your work on this! :beers:

[coveralls]

Coverage increased (+0.001%) to 99.888% when pulling 6e5bc342bdaa6c54175ff714f314a0c616aa502b on felipedau:socks-errors into 0d65a792c97bd2dcb118a939e709d03dd6fad596 on meejah:master.

[felipedau]

Python3: I think if you just "self.message = message or self.message" you'll fix it. This probably shadows the Exception one in Py2, but oh well. And yes, the "recommended" thing is to always str(e) if you wanted the string-representation. I think.

Thanks, did that! SocksError.__init__() has a bit of shadowing/redundancy, but works!

As a result, I had to make a pretty redundant test to make sure this hack does work :P

Tests: yes feel free to change the error-strings in the test asserts. They could even assert on the correct error-class now (e.g. assertTrue(isinstance(ctx.exception, TheSpecificSocksError))) if you want to be "more correct" :) (It is kind of bad to depend on the str() of the Exception, but also I'm pragmatic ;)

Tried to be more correct here, so I pointed to the class attributes / instances where I could.

For SucceedReply lets just delete it. If there's a use-case later it can be easily added. So keep SUCCEEDED I guess that means.

Alright, done!

For the "custom error messages": I'm not sure what you mean? If you think these should be Exception (or similar?) instead of SocksError that's fine too. Or maybe you mean they want their own class too; either way is fine with me I think. They're really "shouldn't happen, but there's weird SOCKS servers out there" errors so I really doubt the calling code can do anything besides tell the user "it didn't work, because 'some technobable'".

Oh no, I was just complaining now we cannot do version_error('message') but have to instantiate a SocksError before making that call. Maybe some hacking to _disconnect could handle both cases (receiving a SocksError and a message) but not sure if that's actually bad. And I do think that these should all be SocksErrors.

Also thanks for your work on this!

I'd say the same! I hope to continue to contribute :)

Let me know what you think. Should I squash the commits? Because it got a bit messy.

Thanks!

[felipedau]

One last thing: now message is None by default, but python's Exception defaults to an empty string.

Should that be changed?

[coveralls]

Coverage increased (+0.001%) to 99.888% when pulling c00fa754402f438a897e96920925a560e4f53aaf on felipedau:socks-errors into 0d65a792c97bd2dcb118a939e709d03dd6fad596 on meejah:master.

[coveralls]

Coverage increased (+0.001%) to 99.888% when pulling c00fa754402f438a897e96920925a560e4f53aaf on felipedau:socks-errors into 0d65a792c97bd2dcb118a939e709d03dd6fad596 on meejah:master.

[coveralls]

Coverage increased (+0.001%) to 99.888% when pulling c00fa754402f438a897e96920925a560e4f53aaf on felipedau:socks-errors into 0d65a792c97bd2dcb118a939e709d03dd6fad596 on meejah:master.

[coveralls]

Coverage increased (+0.001%) to 99.888% when pulling c00fa754402f438a897e96920925a560e4f53aaf on felipedau:socks-errors into 0d65a792c97bd2dcb118a939e709d03dd6fad596 on meejah:master.

[coveralls]

Coverage increased (+0.001%) to 99.888% when pulling c00fa754402f438a897e96920925a560e4f53aaf on felipedau:socks-errors into 0d65a792c97bd2dcb118a939e709d03dd6fad596 on meejah:master.

[felipedau]

For SocksErrors without a message, these changes would make the test fail:

@@ -737,12 +737,14 @@ class SocksErrorTests(unittest.TestCase):
             error = socks._create_socks_error(cls.code)
             self.assertEquals(error.code, cls.code)
             self.assertEquals(error.message, cls.message)
+            self.assertEquals(str(error), cls.message)
             self.assertTrue(isinstance(error, cls))

     def test_custom_error(self):
         def check(e, c, m):
             self.assertEquals(e.code, c)
             self.assertEquals(e.message, m)
+            self.assertEquals(str(e), m)
             self.assertTrue(isinstance(e, socks.SocksError))

         code = 0xFF

test/test_socks.py:747: in check
    self.assertEquals(str(e), m)
../../envs/python3/lib/python3.4/site-packages/twisted/trial/_synctest.py:432: in assertEqual
    super(_Assertions, self).assertEqual(first, second, msg)
E   twisted.trial.unittest.FailTest: 'None' != None

[coveralls]

Coverage increased (+0.001%) to 99.888% when pulling fc04817b2440676c2153ba6abcf158b3d5efa8d8 on felipedau:socks-errors into 0d65a792c97bd2dcb118a939e709d03dd6fad596 on meejah:master.

[coveralls]

Coverage increased (+0.001%) to 99.888% when pulling fc04817b2440676c2153ba6abcf158b3d5efa8d8 on felipedau:socks-errors into 0d65a792c97bd2dcb118a939e709d03dd6fad596 on meejah:master.

[coveralls]

Coverage increased (+0.001%) to 99.888% when pulling fc04817b2440676c2153ba6abcf158b3d5efa8d8 on felipedau:socks-errors into 0d65a792c97bd2dcb118a939e709d03dd6fad596 on meejah:master.

[coveralls]

Coverage increased (+0.001%) to 99.888% when pulling fc04817b2440676c2153ba6abcf158b3d5efa8d8 on felipedau:socks-errors into 0d65a792c97bd2dcb118a939e709d03dd6fad596 on meejah:master.

[felipedau]

Well, I hope these are the last changes. In the end, we made the python3's exceptions look just like python2's and now both have:

'args': ('TTL expired',),
'code': 6,
'message': 'TTL expired',

Some people might not be happy with that, but I don't think it is a big deal. Let me know what you think!

Thanks :)

[coveralls]

Coverage increased (+0.001%) to 99.888% when pulling 939e1356e1bc450112c3a7602369d55c5e63bbcc on felipedau:socks-errors into 0d65a792c97bd2dcb118a939e709d03dd6fad596 on meejah:master.

[meejah]

Great! Looks good :)

I think an empty-string is better than None for a SocksError with no message; is that how the code is right now? As for args, message etc: meh. Python3 users should be doing str(e) anyway...

[felipedau]

Great! Looks good :)

Thanks! Glad we finally made it run on both pythons :)

I think an empty-string is better than None for a SocksError with no message; is that how the code is right now?

I agree. Defaults are: code = None and message = ''.

As for args, message etc: meh. Python3 users should be doing str(e) anyway...

And that should be unnoticeable: .args, .message, and .__str__() all work.

Thanks @meejah!