Open ecederstrand opened 7 years ago
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.
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
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
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
Maybe the memoryview
thing is a Travis issue? The test suite succeeded on my machine.
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
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.
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.
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?
could the maintainer take a look at the PR? @hit9 @lxyu
If we don't reset
self.sock
on connection error, thenmy_socket.is_open()
will still returnTrue
afterTTransportException
is thrown. This may lead consumers to believe theTSocket
can still be used. Example: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.