You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by bgedik <gi...@git.apache.org> on 2018/01/21 17:45:57 UTC
[GitHub] thrift pull request #1476: remove workSocket call that is too early
GitHub user bgedik opened a pull request:
https://github.com/apache/thrift/pull/1476
remove workSocket call that is too early
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 <bg...@...>
Date: 2018-01-21T17:43:49Z
remove workSocket call that is too early
----
---
[GitHub] thrift pull request #1476: Remove premature call to workSocket() in TNonbloc...
Posted by bgedik <gi...@git.apache.org>.
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...
Posted by bgedik <gi...@git.apache.org>.
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...
Posted by bgedik <gi...@git.apache.org>.
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 issue #1476: Remove premature call to workSocket() in TNonblockingSer...
Posted by bgedik <gi...@git.apache.org>.
Github user bgedik commented on the issue:
https://github.com/apache/thrift/pull/1476
@jeking3 This is ready for review now.
---
[GitHub] thrift pull request #1476: Remove premature call to workSocket() in TNonbloc...
Posted by jeking3 <gi...@git.apache.org>.
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 issue #1476: Remove premature call to workSocket() in TNonblockingSer...
Posted by bgedik <gi...@git.apache.org>.
Github user bgedik commented on the issue:
https://github.com/apache/thrift/pull/1476
@jeking3 anything I can do to get this into a mergable state?
---
[GitHub] thrift pull request #1476: Remove premature call to workSocket() in TNonbloc...
Posted by bgedik <gi...@git.apache.org>.
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...
Posted by jeking3 <gi...@git.apache.org>.
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...
Posted by jeking3 <gi...@git.apache.org>.
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...
Posted by jeking3 <gi...@git.apache.org>.
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 issue #1476: Remove premature call to workSocket() in TNonblockingSer...
Posted by bgedik <gi...@git.apache.org>.
Github user bgedik commented on the issue:
https://github.com/apache/thrift/pull/1476
@jeking3 My latest fix worked fine, but failed to compile on windows. I'll rework to something that works for all platforms.
---
[GitHub] thrift pull request #1476: Remove premature call to workSocket() in TNonbloc...
Posted by jeking3 <gi...@git.apache.org>.
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?
---
[GitHub] thrift pull request #1476: Remove premature call to workSocket() in TNonbloc...
Posted by jeking3 <gi...@git.apache.org>.
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...
Posted by bgedik <gi...@git.apache.org>.
Github user bgedik closed the pull request at:
https://github.com/apache/thrift/pull/1476
---
[GitHub] thrift issue #1476: Remove premature call to workSocket() in TNonblockingSer...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1476
Squash to one commit, please.
---
[GitHub] thrift issue #1476: Remove premature call to workSocket() in TNonblockingSer...
Posted by bgedik <gi...@git.apache.org>.
Github user bgedik commented on the issue:
https://github.com/apache/thrift/pull/1476
The following tests FAILED:
16 - concurrency_test (Timeout)
20 - TNonblockingSSLServerTest (Timeout)
I think the failure in {{TNonblockingSSLServerTest}} could be related. concurrency_test passes locally for me, so I am guessing that there may be a different problem with it.
---
[GitHub] thrift issue #1476: Remove premature call to workSocket() in TNonblockingSer...
Posted by bgedik <gi...@git.apache.org>.
Github user bgedik commented on the issue:
https://github.com/apache/thrift/pull/1476
Ok, reverted back.
---
[GitHub] thrift issue #1476: Remove premature call to workSocket() in TNonblockingSer...
Posted by bgedik <gi...@git.apache.org>.
Github user bgedik commented on the issue:
https://github.com/apache/thrift/pull/1476
@jeking3 Here: https://github.com/apache/thrift/pull/1497
---
[GitHub] thrift issue #1476: Remove premature call to workSocket() in TNonblockingSer...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1476
I see now, I may have led you down the wrong path with my comments. So peek() blocks waiting for data the be available to read and the end result of peek is that there is data to read OR the socket disconnected. What you are looking for is a call to see if the socket has data available but does not block. As such, those are different code paths...
Perhaps your original code before this last push was correct. I will take another look.
---
[GitHub] thrift pull request #1476: Remove premature call to workSocket() in TNonbloc...
Posted by bgedik <gi...@git.apache.org>.
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 issue #1476: Remove premature call to workSocket() in TNonblockingSer...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1476
Yes, concurrency_test has sporadic failures - it is a lot better than it used to be; seems to be worse on Visual Studio 10. Perhaps the solution here is to handle EAGAIN properly?
---
[GitHub] thrift pull request #1476: Remove premature call to workSocket() in TNonbloc...
Posted by bgedik <gi...@git.apache.org>.
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 <bg...@...>
Date: 2018-01-21T17:43:49Z
remove workSocket call that is too early
commit e115634a2a9844d08a6d017859e3d838414d2194
Author: Bugra Gedik <bg...@...>
Date: 2018-01-22T00:35:31Z
Make the work socket conditional
commit 8e5ba48c9d62ee4ae79aac5f2cc4f98d2f535866
Author: Bugra Gedik <bg...@...>
Date: 2018-01-22T01:03:04Z
Revert back the changes
commit 017d242479c985a9cb476bcd2da22abaa62ed052
Author: Bugra Gedik <bg...@...>
Date: 2018-01-22T18:52:33Z
Only call workSocket() when there is pending data to read
commit c929047d4a43c3969cc40b204e3717877667edb2
Author: Bugra Gedik <bg...@...>
Date: 2018-01-22T19:29:12Z
Minor update to a comment
commit fb72b94b513f665e49488d8aaee7eda3f4b57a94
Author: Bugra Gedik <bg...@...>
Date: 2018-01-22T20:51:55Z
Only call workSocket() when there is pending data to read
commit 4777cab48641d23bd2246248a07a7a4249a3709f
Author: Bugra Gedik <bg...@...>
Date: 2018-01-22T21:43:04Z
Only call workSocket() when there is pending data to read
commit f69e91be02275c8a06f489030fcda24449329ce2
Author: Bugra Gedik <bg...@...>
Date: 2018-01-22T23:44:10Z
Fix the CMake build
commit 84051190db3234468ffed11a2a61ab8ba6d7e1fd
Author: Bugra Gedik <bg...@...>
Date: 2018-01-22T23:49:27Z
Fix the CMake build
commit 7b60db3a5535195ec6096fea8ead757266d716b5
Author: Bugra Gedik <bg...@...>
Date: 2018-01-22T23:58:34Z
Fix the CMake build
commit 081096091cc828dd7dbcf0962cdaa6c791317e0c
Author: Bugra Gedik <bg...@...>
Date: 2018-01-23T00:14:09Z
Use the correct type for ioctlsocket call
commit 501c4401ddb93cf9bf451dd289d4c3768c0a7b15
Author: Bugra Gedik <bg...@...>
Date: 2018-01-24T19:19:57Z
Make non-blocking peek optional
commit a9dc3f118acb820189696bd13735444ad2d129a3
Author: Bugra Gedik <bg...@...>
Date: 2018-01-24T20:46:40Z
Revert "Make non-blocking peek optional"
This reverts commit 501c4401ddb93cf9bf451dd289d4c3768c0a7b15.
commit c8d422a633a6b848982c48cb769285906f2ea027
Author: Bugra Gedik <bg...@...>
Date: 2018-01-24T21:12:35Z
Review fixes
commit d64a8e0d4709ece4086f054fae7c1c98e3a0ca77
Author: Bugra Gedik <bg...@...>
Date: 2018-01-24T21:15:14Z
Review fixes
commit 83d3c13f09238831e2d06e8b67a018acbcc656c4
Author: Bugra Gedik <bg...@...>
Date: 2018-01-24T21:16:28Z
Review fixes
----
---
[GitHub] thrift pull request #1476: Remove premature call to workSocket() in TNonbloc...
Posted by bgedik <gi...@git.apache.org>.
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...
Posted by bgedik <gi...@git.apache.org>.
Github user bgedik closed the pull request at:
https://github.com/apache/thrift/pull/1476
---