[GitHub] thrift pull request #1476: Remove premature call to workSocket() in TNonbloc...

[bgedik]

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...

[bgedik]

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...

[bgedik]

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...

[bgedik]

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...

[bgedik]

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...

[bgedik]

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...

[jeking3]

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...

[jeking3]

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...

[jeking3]

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...

[bgedik]

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...

[bgedik]

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...

[jeking3]

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...

[jeking3]

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...

[bgedik]

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...

[jeking3]

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?


---