You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2020/07/13 17:23:20 UTC
[GitHub] [thrift] spladug opened a new pull request #2200: THRIFT-5248: Python: Make TSocket.isOpen check if server still connected
spladug opened a new pull request #2200:
URL: https://github.com/apache/thrift/pull/2200
Client: py
This is inspired by changes to the Go library (THRIFT-5214) and, by proxy, [this blog post][1]. The idea is that if the other end of the socket has closed their end of the connection, we can figure that out by doing a non-blocking read on our socket before we waste time serializing and sending a message just to find out the socket is closed when we try to read the response.
I've done some [benchmarking](https://gist.github.com/spladug/91483032d410714e1d416467a476a1ad) on the addition and found it performs reasonably: costing about 2 microseconds per `isOpen()` call.
I've tested on Python 2.7, 3.7, and 3.8 on Linux. I've tested on Python 3.8 on Windows.
We have been running this in production at reddit for several high-throughput services for several days. There was no noticeable impact on latencies but there is a very significant drop in RPC error rates.
[1]: https://github.blog/2020-05-20-three-bugs-in-the-go-mysql-driver/
<!-- Explain the changes in the pull request below: -->
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [x] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? (not required for trivial changes)
- [x] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [x] Did you squash your changes to a single commit? (not required, but preferred)
- [x] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [ ] If your change does not involve any code, include `[skip ci]` anywhere in the commit message to free up build resources.
<!--
The Contributing Guide at:
https://github.com/apache/thrift/blob/master/CONTRIBUTING.md
has more details and tips for committing properly.
-->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] spladug commented on pull request #2200: THRIFT-5248: Python: Make TSocket.isOpen check if server still connected
Posted by GitBox <gi...@apache.org>.
spladug commented on pull request #2200:
URL: https://github.com/apache/thrift/pull/2200#issuecomment-659496879
Thanks @fishy. That should be fixed up now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on a change in pull request #2200: THRIFT-5248: Python: Make TSocket.isOpen check if server still connected
Posted by GitBox <gi...@apache.org>.
fishy commented on a change in pull request #2200:
URL: https://github.com/apache/thrift/pull/2200#discussion_r464500154
##########
File path: lib/py/src/transport/TSocket.py
##########
@@ -74,7 +74,30 @@ def setHandle(self, h):
self.handle = h
def isOpen(self):
- return self.handle is not None
+ if self.handle is None:
+ return False
+
+ # this lets us cheaply see if the other end of the socket is still
+ # connected. if disconnected, we'll get EOF back (expressed as zero
+ # bytes of data) otherwise we'll get one byte or an error indicating
+ # we'd have to block for data.
+ #
+ # note that we're not doing this with socket.MSG_DONTWAIT because 1)
+ # it's linux-specific and 2) gevent-patched sockets hide EAGAIN from us
+ # when timeout is non-zero.
+ original_timeout = self.handle.gettimeout()
+ try:
+ self.handle.settimeout(0)
+ try:
+ peeked_bytes = self.handle.recv(1, socket.MSG_PEEK)
+ except (socket.error, OSError) as exc: # on modern python this is just BlockingIOError
+ if exc.errno == errno.EWOULDBLOCK:
Review comment:
I think we actually need to check both EWOULDBLOCK and EAGAIN here? From the man page of `recv`:
```
EAGAIN or EWOULDBLOCK
The socket is marked nonblocking and the receive operation would block, or a receive timeout had been set and the timeout expired before data was received. POSIX.1 allows either error to be returned for this case, and does not require these constants to have the same value, so a portable application should check for both possibilities.
```
Also should EOF be handled explicitly (return False) instead of just raise again?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] dcelasun commented on pull request #2200: THRIFT-5248: Python: Make TSocket.isOpen check if server still connected
Posted by GitBox <gi...@apache.org>.
dcelasun commented on pull request #2200:
URL: https://github.com/apache/thrift/pull/2200#issuecomment-668092724
Looks good! I don't have a real workload to test this with but as it's working for you guys in prod I think we can merge this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] dcelasun commented on a change in pull request #2200: THRIFT-5248: Python: Make TSocket.isOpen check if server still connected
Posted by GitBox <gi...@apache.org>.
dcelasun commented on a change in pull request #2200:
URL: https://github.com/apache/thrift/pull/2200#discussion_r464510479
##########
File path: lib/py/src/transport/TSocket.py
##########
@@ -74,7 +74,30 @@ def setHandle(self, h):
self.handle = h
def isOpen(self):
- return self.handle is not None
+ if self.handle is None:
+ return False
+
+ # this lets us cheaply see if the other end of the socket is still
+ # connected. if disconnected, we'll get EOF back (expressed as zero
+ # bytes of data) otherwise we'll get one byte or an error indicating
+ # we'd have to block for data.
+ #
+ # note that we're not doing this with socket.MSG_DONTWAIT because 1)
+ # it's linux-specific and 2) gevent-patched sockets hide EAGAIN from us
+ # when timeout is non-zero.
+ original_timeout = self.handle.gettimeout()
+ try:
+ self.handle.settimeout(0)
+ try:
+ peeked_bytes = self.handle.recv(1, socket.MSG_PEEK)
+ except (socket.error, OSError) as exc: # on modern python this is just BlockingIOError
+ if exc.errno == errno.EWOULDBLOCK:
Review comment:
Sounds good!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] dcelasun commented on a change in pull request #2200: THRIFT-5248: Python: Make TSocket.isOpen check if server still connected
Posted by GitBox <gi...@apache.org>.
dcelasun commented on a change in pull request #2200:
URL: https://github.com/apache/thrift/pull/2200#discussion_r464505274
##########
File path: lib/py/src/transport/TSocket.py
##########
@@ -74,7 +74,30 @@ def setHandle(self, h):
self.handle = h
def isOpen(self):
- return self.handle is not None
+ if self.handle is None:
+ return False
+
+ # this lets us cheaply see if the other end of the socket is still
+ # connected. if disconnected, we'll get EOF back (expressed as zero
+ # bytes of data) otherwise we'll get one byte or an error indicating
+ # we'd have to block for data.
+ #
+ # note that we're not doing this with socket.MSG_DONTWAIT because 1)
+ # it's linux-specific and 2) gevent-patched sockets hide EAGAIN from us
+ # when timeout is non-zero.
+ original_timeout = self.handle.gettimeout()
+ try:
+ self.handle.settimeout(0)
+ try:
+ peeked_bytes = self.handle.recv(1, socket.MSG_PEEK)
+ except (socket.error, OSError) as exc: # on modern python this is just BlockingIOError
+ if exc.errno == errno.EWOULDBLOCK:
Review comment:
Yeah, seems like it:
```
If no messages are available at the socket [...] the value -1 is
returned and the external variable errno is set to EAGAIN or EWOULDBLOCK
```
Though I don't know what the behaviour is on Windows.
For EOF, returning is better since `isOpen` didn't throw before and it would be a breaking change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on a change in pull request #2200: THRIFT-5248: Python: Make TSocket.isOpen check if server still connected
Posted by GitBox <gi...@apache.org>.
fishy commented on a change in pull request #2200:
URL: https://github.com/apache/thrift/pull/2200#discussion_r464653402
##########
File path: lib/py/src/transport/TSocket.py
##########
@@ -91,12 +91,13 @@ def isOpen(self):
try:
peeked_bytes = self.handle.recv(1, socket.MSG_PEEK)
except (socket.error, OSError) as exc: # on modern python this is just BlockingIOError
- if exc.errno == errno.EWOULDBLOCK:
+ if exc.errno in (errno.EWOULDBLOCK, errno.EAGAIN):
Review comment:
Looks like this would be fine:
```
>>> foo = 1
>>> foo in (1, 1, 2, 3)
True
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy merged pull request #2200: THRIFT-5248: Python: Make TSocket.isOpen check if server still connected
Posted by GitBox <gi...@apache.org>.
fishy merged pull request #2200:
URL: https://github.com/apache/thrift/pull/2200
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on pull request #2200: THRIFT-5248: Python: Make TSocket.isOpen check if server still connected
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2200:
URL: https://github.com/apache/thrift/pull/2200#issuecomment-659499781
Looks good to me, but I'm less familiar with the python library code. @Jens-G I think you reviewed most of the recent python library code changes, could you please take a look when you got time?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on a change in pull request #2200: THRIFT-5248: Python: Make TSocket.isOpen check if server still connected
Posted by GitBox <gi...@apache.org>.
fishy commented on a change in pull request #2200:
URL: https://github.com/apache/thrift/pull/2200#discussion_r464509741
##########
File path: lib/py/src/transport/TSocket.py
##########
@@ -74,7 +74,30 @@ def setHandle(self, h):
self.handle = h
def isOpen(self):
- return self.handle is not None
+ if self.handle is None:
+ return False
+
+ # this lets us cheaply see if the other end of the socket is still
+ # connected. if disconnected, we'll get EOF back (expressed as zero
+ # bytes of data) otherwise we'll get one byte or an error indicating
+ # we'd have to block for data.
+ #
+ # note that we're not doing this with socket.MSG_DONTWAIT because 1)
+ # it's linux-specific and 2) gevent-patched sockets hide EAGAIN from us
+ # when timeout is non-zero.
+ original_timeout = self.handle.gettimeout()
+ try:
+ self.handle.settimeout(0)
+ try:
+ peeked_bytes = self.handle.recv(1, socket.MSG_PEEK)
+ except (socket.error, OSError) as exc: # on modern python this is just BlockingIOError
+ if exc.errno == errno.EWOULDBLOCK:
Review comment:
Ah right, I forgot the old implementation of isOpen is just a simple check that would never raise.
In that case, I think we need to `return False` explicitly on all other errors, not only EOF.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on pull request #2200: THRIFT-5248: Python: Make TSocket.isOpen check if server still connected
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2200:
URL: https://github.com/apache/thrift/pull/2200#issuecomment-659485095
From Travis CI output (https://travis-ci.org/github/apache/thrift/jobs/707716276):
```
# Python code style
flake8
./lib/py/test/test_socket.py:3:1: F401 'logging' imported but unused
```
Other CI failures are from the known broken/flaky cross tests between java-erl.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on a change in pull request #2200: THRIFT-5248: Python: Make TSocket.isOpen check if server still connected
Posted by GitBox <gi...@apache.org>.
fishy commented on a change in pull request #2200:
URL: https://github.com/apache/thrift/pull/2200#discussion_r464500154
##########
File path: lib/py/src/transport/TSocket.py
##########
@@ -74,7 +74,30 @@ def setHandle(self, h):
self.handle = h
def isOpen(self):
- return self.handle is not None
+ if self.handle is None:
+ return False
+
+ # this lets us cheaply see if the other end of the socket is still
+ # connected. if disconnected, we'll get EOF back (expressed as zero
+ # bytes of data) otherwise we'll get one byte or an error indicating
+ # we'd have to block for data.
+ #
+ # note that we're not doing this with socket.MSG_DONTWAIT because 1)
+ # it's linux-specific and 2) gevent-patched sockets hide EAGAIN from us
+ # when timeout is non-zero.
+ original_timeout = self.handle.gettimeout()
+ try:
+ self.handle.settimeout(0)
+ try:
+ peeked_bytes = self.handle.recv(1, socket.MSG_PEEK)
+ except (socket.error, OSError) as exc: # on modern python this is just BlockingIOError
+ if exc.errno == errno.EWOULDBLOCK:
Review comment:
I think we actually need to check both EWOULDBLOCK and EAGAIN here? From the man page of `recv`:
```
EAGAIN or EWOULDBLOCK
The socket is marked nonblocking and the receive operation would block, or a receive timeout had been set and the timeout expired before data was received. POSIX.1 allows either error to be returned for this case, and does not require these constants to have the same value, so a portable application should check for both possibilities.
```
Also should EOF be handled explicitly (return False) instead of just raise again?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on pull request #2200: THRIFT-5248: Python: Make TSocket.isOpen check if server still connected
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2200:
URL: https://github.com/apache/thrift/pull/2200#issuecomment-668261636
@spladug Can you squash the commits so I can merge it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] spladug commented on a change in pull request #2200: THRIFT-5248: Python: Make TSocket.isOpen check if server still connected
Posted by GitBox <gi...@apache.org>.
spladug commented on a change in pull request #2200:
URL: https://github.com/apache/thrift/pull/2200#discussion_r464523036
##########
File path: lib/py/src/transport/TSocket.py
##########
@@ -74,7 +74,30 @@ def setHandle(self, h):
self.handle = h
def isOpen(self):
- return self.handle is not None
+ if self.handle is None:
+ return False
+
+ # this lets us cheaply see if the other end of the socket is still
+ # connected. if disconnected, we'll get EOF back (expressed as zero
+ # bytes of data) otherwise we'll get one byte or an error indicating
+ # we'd have to block for data.
+ #
+ # note that we're not doing this with socket.MSG_DONTWAIT because 1)
+ # it's linux-specific and 2) gevent-patched sockets hide EAGAIN from us
+ # when timeout is non-zero.
+ original_timeout = self.handle.gettimeout()
+ try:
+ self.handle.settimeout(0)
+ try:
+ peeked_bytes = self.handle.recv(1, socket.MSG_PEEK)
+ except (socket.error, OSError) as exc: # on modern python this is just BlockingIOError
+ if exc.errno == errno.EWOULDBLOCK:
Review comment:
I'll add both and make it swallow the unexpected exception.
FWIW, looks like the most recent OS to have EAGAIN != EWOULDBLOCK is quite old: https://stackoverflow.com/questions/7003234/which-systems-define-eagain-and-ewouldblock-as-different-values/7003379#7003379 but it makes sense to be most portable here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] spladug commented on a change in pull request #2200: THRIFT-5248: Python: Make TSocket.isOpen check if server still connected
Posted by GitBox <gi...@apache.org>.
spladug commented on a change in pull request #2200:
URL: https://github.com/apache/thrift/pull/2200#discussion_r464656554
##########
File path: lib/py/src/transport/TSocket.py
##########
@@ -91,12 +91,13 @@ def isOpen(self):
try:
peeked_bytes = self.handle.recv(1, socket.MSG_PEEK)
except (socket.error, OSError) as exc: # on modern python this is just BlockingIOError
- if exc.errno == errno.EWOULDBLOCK:
+ if exc.errno in (errno.EWOULDBLOCK, errno.EAGAIN):
return True
- raise
+ return False
Review comment:
What kinds of errors are you concerned about? An over-broad catch like that would mask if someone made a legitimate bug in the covered code. AFAIK even in 2.7 `socket.error` should catch everything actually socket related: https://docs.python.org/2/library/socket.html#socket.error
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on a change in pull request #2200: THRIFT-5248: Python: Make TSocket.isOpen check if server still connected
Posted by GitBox <gi...@apache.org>.
fishy commented on a change in pull request #2200:
URL: https://github.com/apache/thrift/pull/2200#discussion_r464500154
##########
File path: lib/py/src/transport/TSocket.py
##########
@@ -74,7 +74,30 @@ def setHandle(self, h):
self.handle = h
def isOpen(self):
- return self.handle is not None
+ if self.handle is None:
+ return False
+
+ # this lets us cheaply see if the other end of the socket is still
+ # connected. if disconnected, we'll get EOF back (expressed as zero
+ # bytes of data) otherwise we'll get one byte or an error indicating
+ # we'd have to block for data.
+ #
+ # note that we're not doing this with socket.MSG_DONTWAIT because 1)
+ # it's linux-specific and 2) gevent-patched sockets hide EAGAIN from us
+ # when timeout is non-zero.
+ original_timeout = self.handle.gettimeout()
+ try:
+ self.handle.settimeout(0)
+ try:
+ peeked_bytes = self.handle.recv(1, socket.MSG_PEEK)
+ except (socket.error, OSError) as exc: # on modern python this is just BlockingIOError
+ if exc.errno == errno.EWOULDBLOCK:
Review comment:
I think we actually need to check both EWOULDBLOCK and EAGAIN here? From the man page of `recv`:
```
EAGAIN or EWOULDBLOCK
The socket is marked nonblocking and the receive operation would block, or a receive timeout had been set and the timeout expired before data was received. POSIX.1 allows either error to be returned for this case, and does not require these constants to have the same value, so a portable application should check for both possibilities.
```
~Also should EOF be handled explicitly (return False) instead of just raise again?~ nevermind. I think before this implementation we would also raise EOF errors when that happens.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on a change in pull request #2200: THRIFT-5248: Python: Make TSocket.isOpen check if server still connected
Posted by GitBox <gi...@apache.org>.
fishy commented on a change in pull request #2200:
URL: https://github.com/apache/thrift/pull/2200#discussion_r464652291
##########
File path: lib/py/src/transport/TSocket.py
##########
@@ -91,12 +91,13 @@ def isOpen(self):
try:
peeked_bytes = self.handle.recv(1, socket.MSG_PEEK)
except (socket.error, OSError) as exc: # on modern python this is just BlockingIOError
- if exc.errno == errno.EWOULDBLOCK:
+ if exc.errno in (errno.EWOULDBLOCK, errno.EAGAIN):
Review comment:
Would this cause trouble if `errno.EWOULDBLOCK` and `errno.EAGAIN` is the same thing? I assume it won't but just to double check here :)
##########
File path: lib/py/src/transport/TSocket.py
##########
@@ -91,12 +91,13 @@ def isOpen(self):
try:
peeked_bytes = self.handle.recv(1, socket.MSG_PEEK)
except (socket.error, OSError) as exc: # on modern python this is just BlockingIOError
- if exc.errno == errno.EWOULDBLOCK:
+ if exc.errno in (errno.EWOULDBLOCK, errno.EAGAIN):
return True
- raise
+ return False
Review comment:
I think we also need to catch exceptions that's not `socket.error` nor `OSError` to return False, so maybe it's better to have a catch-all on the upper level of try...finally to just `return False` for all exceptions? I know catch-all is usually discouraged in python linters but am not sure if there's a better approach here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org