[GitHub] thrift pull request #1476: Remove premature call to workSocket() in TNonbloc...
GitHub user bgedik reopened a pull request: https://github.com/apache/thrift/pull/1476 Remove premature call to workSocket() in TNonblockingServer You can merge this pull request into a Git repository by running: $ git pull https://github.com/bgedik/thrift bugfix/non-blocking-server-calls-workSocket-too-early Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1476.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1476 commit f3c682e9c41d67d8cd1ab2d75f6259b6e0af40da Author: Bugra Gedik Date: 2018-01-21T17:43:49Z remove workSocket call that is too early commit e115634a2a9844d08a6d017859e3d838414d2194 Author: Bugra Gedik Date: 2018-01-22T00:35:31Z Make the work socket conditional commit 8e5ba48c9d62ee4ae79aac5f2cc4f98d2f535866 Author: Bugra Gedik Date: 2018-01-22T01:03:04Z Revert back the changes commit 017d242479c985a9cb476bcd2da22abaa62ed052 Author: Bugra Gedik Date: 2018-01-22T18:52:33Z Only call workSocket() when there is pending data to read commit c929047d4a43c3969cc40b204e3717877667edb2 Author: Bugra Gedik Date: 2018-01-22T19:29:12Z Minor update to a comment commit fb72b94b513f665e49488d8aaee7eda3f4b57a94 Author: Bugra Gedik Date: 2018-01-22T20:51:55Z Only call workSocket() when there is pending data to read commit 4777cab48641d23bd2246248a07a7a4249a3709f Author: Bugra Gedik Date: 2018-01-22T21:43:04Z Only call workSocket() when there is pending data to read commit f69e91be02275c8a06f489030fcda24449329ce2 Author: Bugra Gedik Date: 2018-01-22T23:44:10Z Fix the CMake build commit 84051190db3234468ffed11a2a61ab8ba6d7e1fd Author: Bugra Gedik Date: 2018-01-22T23:49:27Z Fix the CMake build commit 7b60db3a5535195ec6096fea8ead757266d716b5 Author: Bugra Gedik Date: 2018-01-22T23:58:34Z Fix the CMake build commit 081096091cc828dd7dbcf0962cdaa6c791317e0c Author: Bugra Gedik Date: 2018-01-23T00:14:09Z Use the correct type for ioctlsocket call commit 501c4401ddb93cf9bf451dd289d4c3768c0a7b15 Author: Bugra Gedik Date: 2018-01-24T19:19:57Z Make non-blocking peek optional commit a9dc3f118acb820189696bd13735444ad2d129a3 Author: Bugra Gedik Date: 2018-01-24T20:46:40Z Revert "Make non-blocking peek optional" This reverts commit 501c4401ddb93cf9bf451dd289d4c3768c0a7b15. commit c8d422a633a6b848982c48cb769285906f2ea027 Author: Bugra Gedik Date: 2018-01-24T21:12:35Z Review fixes commit d64a8e0d4709ece4086f054fae7c1c98e3a0ca77 Author: Bugra Gedik Date: 2018-01-24T21:15:14Z Review fixes commit 83d3c13f09238831e2d06e8b67a018acbcc656c4 Author: Bugra Gedik Date: 2018-01-24T21:16:28Z Review fixes ---
[GitHub] thrift pull request #1476: Remove premature call to workSocket() in TNonbloc...
Github user bgedik closed the pull request at: https://github.com/apache/thrift/pull/1476 ---
[GitHub] thrift pull request #1476: Remove premature call to workSocket() in TNonbloc...
Github user bgedik closed the pull request at: https://github.com/apache/thrift/pull/1476 ---
[GitHub] thrift pull request #1476: Remove premature call to workSocket() in TNonbloc...
Github user bgedik commented on a diff in the pull request: https://github.com/apache/thrift/pull/1476#discussion_r163681677 --- Diff: lib/cpp/src/thrift/transport/TSSLSocket.h --- @@ -78,6 +78,7 @@ class TSSLSocket : public TSocket { bool peek(); void open(); void close(); + bool hasPendingDataToRead(); --- End diff -- Sam applies to ``read``. This goes back to my original point. TSocket should behave differently for blocking vs non-blocking socket. Look at how ``TSocket`` and ``TSSLSocket`` handle ``read``s on a non-blocking socket. They throw exceptions. Luckily for me, when not using SSL, ``TNonBlockingServer`` does not ever call ``read`` on a socket that is not ready to consume data. However, that is not the case for ``SSL``. Read calls in ``TNonBlockingServer`` are retried by catching the exception and searching for ``"retry"`` in the exception message. The correct way to fix all this mess is to make ``TSocket`` aware of whether the socket is blocking and non-blocking and behave accordingly. Converting EAGAIN to an exception in the case of non-blocking sockets is a major performance hit. My suggesting is to handle the issue of making ``TSocket`` and ``TSSLSocket`` aware of the blocking/non-blocking nature of the socket in a separate Jira issue. ---
[GitHub] thrift pull request #1476: Remove premature call to workSocket() in TNonbloc...
Github user bgedik commented on a diff in the pull request: https://github.com/apache/thrift/pull/1476#discussion_r163679874 --- Diff: lib/cpp/src/thrift/transport/TSSLSocket.cpp --- @@ -249,6 +249,17 @@ TSSLSocket::~TSSLSocket() { close(); } +bool TSSLSocket::hasPendingDataToRead() { + if (!isOpen()) { +return false; + } + initializeHandshake(); + if (!checkHandshake()) +throw TSSLException("SSL_peek: Handshake is not completed"); + // data may be available in SSL buffers (note: SSL_pending does not have a failure mode) + return TSocket::hasPendingDataToRead() || SSL_pending(ssl_) > 0; --- End diff -- Right. Nice catch. ---
[GitHub] thrift pull request #1476: Remove premature call to workSocket() in TNonbloc...
Github user bgedik commented on a diff in the pull request: https://github.com/apache/thrift/pull/1476#discussion_r163673632 --- Diff: lib/cpp/src/thrift/transport/TSSLSocket.cpp --- @@ -249,6 +249,17 @@ TSSLSocket::~TSSLSocket() { close(); } +bool TSSLSocket::hasPendingDataToRead() { + if (!isOpen()) { +return false; + } + initializeHandshake(); + if (!checkHandshake()) +throw TSSLException("SSL_peek: Handshake is not completed"); --- End diff -- Fixed. ---
[GitHub] thrift pull request #1476: Remove premature call to workSocket() in TNonbloc...
Github user jeking3 commented on a diff in the pull request: https://github.com/apache/thrift/pull/1476#discussion_r163663375 --- Diff: lib/cpp/src/thrift/transport/TSSLSocket.cpp --- @@ -249,6 +249,17 @@ TSSLSocket::~TSSLSocket() { close(); } +bool TSSLSocket::hasPendingDataToRead() { + if (!isOpen()) { +return false; + } + initializeHandshake(); + if (!checkHandshake()) +throw TSSLException("SSL_peek: Handshake is not completed"); --- End diff -- This should probably say something other than SSL_peek? ---
[GitHub] thrift pull request #1476: Remove premature call to workSocket() in TNonbloc...
Github user jeking3 commented on a diff in the pull request: https://github.com/apache/thrift/pull/1476#discussion_r163664502 --- Diff: lib/cpp/src/thrift/transport/TSSLSocket.h --- @@ -78,6 +78,7 @@ class TSSLSocket : public TSocket { bool peek(); void open(); void close(); + bool hasPendingDataToRead(); --- End diff -- I think my comments were wrong; peek blocks until there is something to do when the socket is a blocking socket. I think that if the socket knew it was non-blocking then a call to peek() would behave like a call to hasPendingDataToRead, i.e. it would be a non-blocking call. Perhaps that's a better way to approach it, but I would have to look at the code a little more closely. I think the original intention of my comment was correct, there should be only one way to peek. Calling peek() on a non-blocking socket should not block; calling peek() on a blocking socket should block. ---
[GitHub] thrift pull request #1476: Remove premature call to workSocket() in TNonbloc...
Github user jeking3 commented on a diff in the pull request: https://github.com/apache/thrift/pull/1476#discussion_r163663584 --- Diff: lib/cpp/src/thrift/transport/TSSLSocket.cpp --- @@ -249,6 +249,17 @@ TSSLSocket::~TSSLSocket() { close(); } +bool TSSLSocket::hasPendingDataToRead() { + if (!isOpen()) { +return false; + } + initializeHandshake(); + if (!checkHandshake()) +throw TSSLException("SSL_peek: Handshake is not completed"); + // data may be available in SSL buffers (note: SSL_pending does not have a failure mode) + return TSocket::hasPendingDataToRead() || SSL_pending(ssl_) > 0; --- End diff -- Should SSL_pending be checked first for efficiency? First check the SSL buffers, then if those are clear then check the socket. ---
[GitHub] thrift pull request #1476: Remove premature call to workSocket() in TNonbloc...
Github user bgedik commented on a diff in the pull request: https://github.com/apache/thrift/pull/1476#discussion_r163651048 --- Diff: lib/cpp/src/thrift/transport/TSSLSocket.h --- @@ -78,6 +78,7 @@ class TSSLSocket : public TSocket { bool peek(); void open(); void close(); + bool hasPendingDataToRead(); --- End diff -- I implemented option #2. In terms of your matrix choice suggestion: How can we make that happen? Is that something I can help with? I am bothered by the TNonblockingSeverTest not catching the problem reported in this bug. The moment we switched to 0.11, it started throwing unexpected exceptions for us, when using Java client against C++ server. This should have been caught by the existing tests, but it did not happen. I would like to help with this. So if you have a suggestion for me, please let me know. ---
[GitHub] thrift pull request #1476: Remove premature call to workSocket() in TNonbloc...
Github user bgedik commented on a diff in the pull request: https://github.com/apache/thrift/pull/1476#discussion_r163640014 --- Diff: lib/cpp/src/thrift/transport/TSSLSocket.h --- @@ -78,6 +78,7 @@ class TSSLSocket : public TSocket { bool peek(); void open(); void close(); + bool hasPendingDataToRead(); --- End diff -- @jeking3 You said it would be preferable to have one way to ask, "is there any data I can read" that does not block. I completely agree. But the existing ``peek`` method is not designed to be non-blocking. So I have a few choices here: * Update the doxygen comment of ``peek`` to reflect what it does and keep ``hasPendingDataToRead`` * Add an optional argument to peek that says ``nonBlocking=false`` and if it is provided as ``true``, do what ``hasPendingDataToRead`` used to do and remove ``hasPendingDataToRead``. * Change ``peek`` to be always non-blocking. How about #2 above? I think #3 is not easy, as it will require code changes in other places that I am not comfortable with. ---
[GitHub] thrift pull request #1476: Remove premature call to workSocket() in TNonbloc...
Github user jeking3 commented on a diff in the pull request: https://github.com/apache/thrift/pull/1476#discussion_r163623295 --- Diff: lib/cpp/src/thrift/transport/TSSLSocket.h --- @@ -78,6 +78,7 @@ class TSSLSocket : public TSocket { bool peek(); void open(); void close(); + bool hasPendingDataToRead(); --- End diff -- Note that the cross test suite currently does not test asynchronous. It is more of a protocol test; it would be interesting to add one more matrix choice of threaded vs. nonblocking for the languages that support it, and test both against both. ---
[GitHub] thrift pull request #1476: Remove premature call to workSocket() in TNonbloc...
Github user jeking3 commented on a diff in the pull request: https://github.com/apache/thrift/pull/1476#discussion_r163622772 --- Diff: lib/cpp/src/thrift/transport/TSSLSocket.h --- @@ -78,6 +78,7 @@ class TSSLSocket : public TSocket { bool peek(); void open(); void close(); + bool hasPendingDataToRead(); --- End diff -- It would be preferable to have one way to ask, "is there any data I can read" that does not block. ---
[GitHub] thrift pull request #1476: Remove premature call to workSocket() in TNonbloc...
Github user bgedik commented on a diff in the pull request: https://github.com/apache/thrift/pull/1476#discussion_r163420333 --- Diff: lib/cpp/src/thrift/transport/TSSLSocket.h --- @@ -78,6 +78,7 @@ class TSSLSocket : public TSocket { bool peek(); void open(); void close(); + bool hasPendingDataToRead(); --- End diff -- Peek is designed to be blocking for blocking sockets, non-blocking for non-blocking sockets. This one is non-blocking for both. I can replace the existing peek() with this implementation if you prefer that. ---
[GitHub] thrift pull request #1476: Remove premature call to workSocket() in TNonbloc...
Github user jeking3 commented on a diff in the pull request: https://github.com/apache/thrift/pull/1476#discussion_r163418365 --- Diff: lib/cpp/src/thrift/transport/TSSLSocket.h --- @@ -78,6 +78,7 @@ class TSSLSocket : public TSocket { bool peek(); void open(); void close(); + bool hasPendingDataToRead(); --- End diff -- Isn't this what peek() is for? ---