Reset self.sock if connection fails

[ecederstrand]

If we don't reset self.sock on connection error, then my_socket.is_open() will still return True after TTransportException is thrown. This may lead consumers to believe the TSocket can still be used. Example:

>>> from thriftpy.transport import TSocket
>>> t = TSocket(host='google.com', port=7777)
>>> t.is_open()
False
>>> t.open()
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/thriftpy/transport/socket.py", line 96, in open
    self.sock.connect(addr)
ConnectionRefusedError: [Errno 111] Connection refused

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3/dist-packages/thriftpy/transport/socket.py", line 104, in open
    message="Could not connect to %s" % str(addr))
thriftpy.transport.TTransportException: TTransportException(type=1, message="Could not connect to ('google.com', 7777)")
>>> t.is_open()
True

The code that lead to this was more complicated, of course, but the point is that without this patch, consumers need to be very careful with error handling.

[ecederstrand]

Tests are failing, but only on Python 2.6. One of the failures seem to be NameError: global name 'memoryview' is not defined but I'm not sure if this is actually related to my patch. I'm a newcomer to thriftpy, so It'd be great if someone more familiar with the codebase can have a look.

[ecederstrand]

An additional surprise. After attempting to close a failing socket, it's still reported as open:

>>> from thriftpy.transport import TSocket
>>> t = TSocket(host='google.com', port=7777)
>>> t.open()
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/thriftpy/transport/socket.py", line 96, in open
    self.sock.connect(addr)
ConnectionRefusedError: [Errno 111] Connection refused

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3/dist-packages/thriftpy/transport/socket.py", line 104, in open
    message="Could not connect to %s" % str(addr))
thriftpy.transport.TTransportException: TTransportException(type=1, message="Could not connect to ('google.com', 7777)")
>>> t.close()
>>> t.is_open()
True

[wbolster]

i think these changes make a lot of sense and might help solve some "mysterious" happybase (thriftpy user) connection related errors.

[ecederstrand]

What is needed to get this in? Should I convert the examples above to unit tests and add them to https://github.com/eleme/thriftpy/blob/develop/tests/test_socket.py? EDIT: tests committed

[wbolster]

the travis build failed on py26, but i am not sure it has anything to do with these changes:

E                   NameError: global name 'memoryview' is not defined

[ecederstrand]

Maybe the memoryview thing is a Travis issue? The test suite succeeded on my machine.

[wbolster]

didn't look closely, but the main reason is that python 2.6 does not have it:

$ python2.6
Python 2.6.9 (default, Mar  6 2016, 02:31:36) 
[GCC 5.3.1 20160225] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> memoryview
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'memoryview' is not defined

[ecederstrand]

Ok, the memoryview problem pops up because setup.py requires tornado version >=4.0 and < 5.0. tornado dropped support for Python 2.6 in 4.4.0 (http://www.tornadoweb.org/en/stable/releases/v4.4.0.html) but pip installs tornado==4.5.1.

Essentially, thriftpy no longer supports python 2.6 with the curent setup.py. Also, the Travis log has this: DEPRECATION: Python 2.6 is no longer supported by the Python core team, please upgrade your Python. A future version of pip will drop support for Python 2.6.

So, is it time for thriftpy to drop support for Python2.6? I'll happily do it here to make Travis happy, but I think it's better suited for a separate PR.

[ecederstrand]

Some (all?) pypy tests related to unix sockets are failing with TTransportException(message='Could not connect to /tmp/thriftpy_test.sock', type=1). I have no idea how that could be related to this patch. The same tests succeed on all other Python versions.

[wbolster]

ok i would say this is good to go in as it stands now.

but i am not a maintainer so perhaps @hit9 or @lxyu (recently active maintainers) can have a look?

[bunnyc1986]

could the maintainer take a look at the PR?
@hit9 @lxyu