[GitHub] thrift pull request #1497: Do not call workSocket() in TNonblockigServer wit...
Github user asfgit closed the pull request at: https://github.com/apache/thrift/pull/1497 ---
[GitHub] thrift pull request #1497: Do not call workSocket() in TNonblockigServer wit...
Github user bgedik commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1497#discussion_r172754060
--- Diff: lib/cpp/src/thrift/server/TNonblockingServer.cpp ---
@@ -472,6 +472,18 @@ void TNonblockingServer::TConnection::workSocket() {
}
// size known; now get the rest of the frame
transition();
+
+// If the socket has more data than the frame header, continue to work
on it. This is not strictly necessary for
+// regular sockets, because if there is more data, libevent will fire
the event handler registered for read
+// readiness, which will in turn call workSocket(). However, some
socket types (such as TSSLSocket) may have the
+// data sitting in their internal buffers and from libevent's
perspective, there is no further data available. In
+// that case, not having this workSocket() call here would result in a
hang as we will never get to work the socket,
+// despite having more data.
+if (tSocket_->hasPendingDataToRead())
--- End diff --
It can't. Note that the line above is ``transition()``. So the next time we
call ``workSocket()`` it is a different state.
Btw, I assume you notice that the original ``workSocket`` call I removed,
which was causing the superfluous exception, was inside ``workSocket`` as well.
---
[GitHub] thrift pull request #1497: Do not call workSocket() in TNonblockigServer wit...
Github user bgedik commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1497#discussion_r172752961
--- Diff: lib/cpp/src/thrift/server/TNonblockingServer.cpp ---
@@ -472,6 +472,18 @@ void TNonblockingServer::TConnection::workSocket() {
}
// size known; now get the rest of the frame
transition();
+
+// If the socket has more data than the frame header, continue to work
on it. This is not strictly necessary for
+// regular sockets, because if there is more data, libevent will fire
the event handler registered for read
+// readiness, which will in turn call workSocket(). However, some
socket types (such as TSSLSocket) may have the
+// data sitting in their internal buffers and from libevent's
perspective, there is no further data available. In
+// that case, not having this workSocket() call here would result in a
hang as we will never get to work the socket,
+// despite having more data.
+if (tSocket_->hasPendingDataToRead())
--- End diff --
It should be an ``if``. At this point, we know the frame size. The next
step is to read the frame data itself. And for that, there is already logic to
work the socket until all the data is read. For SSL specifically, there is no
concern that the SSL buffers can keep pending data forever, because when the
entire data frame arrives, there won't be any incomplete SSL bytes. In any
case, the code should work generically for all socket types.
---
[GitHub] thrift pull request #1497: Do not call workSocket() in TNonblockigServer wit...
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1497#discussion_r172695014
--- Diff: lib/cpp/src/thrift/server/TNonblockingServer.cpp ---
@@ -472,6 +472,18 @@ void TNonblockingServer::TConnection::workSocket() {
}
// size known; now get the rest of the frame
transition();
+
+// If the socket has more data than the frame header, continue to work
on it. This is not strictly necessary for
+// regular sockets, because if there is more data, libevent will fire
the event handler registered for read
+// readiness, which will in turn call workSocket(). However, some
socket types (such as TSSLSocket) may have the
+// data sitting in their internal buffers and from libevent's
perspective, there is no further data available. In
+// that case, not having this workSocket() call here would result in a
hang as we will never get to work the socket,
+// despite having more data.
+if (tSocket_->hasPendingDataToRead())
--- End diff --
I am concerned this can cause an infinite loop if data is always available,
since it calls itself.
---
[GitHub] thrift pull request #1497: Do not call workSocket() in TNonblockigServer wit...
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1497#discussion_r172694884
--- Diff: lib/cpp/src/thrift/server/TNonblockingServer.cpp ---
@@ -472,6 +472,18 @@ void TNonblockingServer::TConnection::workSocket() {
}
// size known; now get the rest of the frame
transition();
+
+// If the socket has more data than the frame header, continue to work
on it. This is not strictly necessary for
+// regular sockets, because if there is more data, libevent will fire
the event handler registered for read
+// readiness, which will in turn call workSocket(). However, some
socket types (such as TSSLSocket) may have the
+// data sitting in their internal buffers and from libevent's
perspective, there is no further data available. In
+// that case, not having this workSocket() call here would result in a
hang as we will never get to work the socket,
+// despite having more data.
+if (tSocket_->hasPendingDataToRead())
--- End diff --
Should this be an "if" or a "while"?
---
