[jira] [Commented] (THRIFT-3084) C++ add concurrent client limit to threaded servers

[James E. King, III (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-3084?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14490814#comment-14490814
 ] 

James E. King, III commented on THRIFT-3084:


Hi Randy,

I do understand what you are saying however I must respectfully disagree with 
the argument.  Please take a look at the files I posted in THRIFT-3083.  I 
think the elegance of boiling down a server to the implementation of a client 
connected, a client disconnected, and the ability to add additional 
post-conditions to serve() is exactly what C++ was intended for... I think what 
you are arguing for is the ability to subclass the servers as well as make the 
servers understandable to junior C++ engineers.  I feel that it is extremely 
important to have consistent serve() and client handling behavior which is why 
I introduced 3081 and 3083 into Jira.

My biggest concern with not doing this work is that the different framework 
servers will behave in different ways even though they intend to provide the 
same post-conditons.  Case in point, if you look at the pre-THRIFT-3083 
TThreadPoolServer::serve() handling if TTransportException, is is quite 
different from TSimpleServer and TThreadedSevrer, when in fact it wants to be 
the same since there is only on TServerTransport, and there should be 
consistent exception behavior for TServerTransport no matter what is consuming 
it.  One logs some issues and one does not.  In another case, TSimpleServer 
will attempt to close the client if an error occurs, but the TThreadedServer 
only attempts to close the input and output protocol transports and makes to 
attempt to close the client. Both subclasses need to enforce the same setof 
post-conditions so why do they differ/  They differ because identical code was 
copied and modified instad of being consolidated.

With the introduction of THRIFT-2441 I had to modify three implementations of 
TServer to ensure they were properly handling TTransportException(INTERRUPTED). 
 I think it makes sense to have a single instantiation of the server serve() 
and a single instantiation of the client run loop since the generated code 
depends on reliable interface contracts, as does the TServerTransport.

Given the processing logic in a TSimpleServer and a TThread*Server wants to be 
identical ffor a given client, this is why I introduced THRIFT-3081.  On 
further observation is was clear that the three sevrers save the non-blocking 
server had extremely similar logic patterns, so it made sense to me to 
consolidate and {{standardize}} their behavior so that folks using the 
framework could experience consistent behavior when switch between server types 
provided by the framework.  I belive the combination of 2441, 3081, 3083, and 
3084 will yield a minimum viable production quality server provided by the 
framework proper, where folks who want to use Thrift will no longer need to 
roll their own TSevrer to achieve enterprise quality behavior.  This should 
encourage adoption and reduce intergation effort.  Given other projects like 
protobuf only provide the protocol portion, enhancing the transport and server 
should further distance the project as useful from alternatives.

One thing I absolutely love about the original concept of thrift was the clear 
separation of processor, protocol and transport.  I find this to be unique 
within the field and the changes I have made [hopefully] do not violate those 
core axioms.  TO answer your question - yes, TSimpleServer and any other 
TServer including TThreadPoolServer should handle clients in a consistent 
manner.  The differences between the servers has been boiled down to the 
following three items:

1. How do I handle a new client connection?
2. How do I handle a client that has disconnected?
3. How to I ensure that a post-condition of serve() is that no clients are 
connected?

The changes in THRIFT-3083 make this very clear and easy to do.

In argument to your statement that TSimpleServer be self-contained and distinct 
from other servers, , if TSimpleServer and TThreadedServer have 90% common code 
and 10% divergent code in the respect of items 1 and 3 above, why should the 
common code be replicated such tht is has a chance to diverge in behavior?  It 
should notbe different, because it is in fact the same, and I would encourage 
any C++ engineer to avoid code duplication with the same set of changes that I 
have submitted to the project, because in the end when code is not duplicated, 
it only needs to be tested once to ensure it is functioning properly.

TServerTransport makes it easy (apart from the crazy constructor variances) for 
anyone to make a server that differs in the areas where it matters most - the 
three items I listed as 1,2,3 previously.

Thrift-3084 is specifically to ensure we do not accept() more clients than the 
server allows; THRIFT-3083 is the right place to discuss TServer 

[jira] [Commented] (THRIFT-3084) C++ add concurrent client limit to threaded servers

[James E. King, III (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-3084?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14490284#comment-14490284
 ] 

James E. King, III commented on THRIFT-3084:


I believe that instead of modifying TServerTransport and inserting a barrier 
into TServerSocket, I can instead implement a Semaphore class and have the 
TThreadedServer itself block the serve() thread after accepting a client that 
puts the server at the concurrent client limit.  I like not having to modify 
transport in this case.

 C++ add concurrent client limit to threaded servers
 ---

 Key: THRIFT-3084
 URL: https://issues.apache.org/jira/browse/THRIFT-3084
 Project: Thrift
  Issue Type: Improvement
  Components: C++ - Library
Affects Versions: 0.8, 0.9, 0.9.1, 0.9.2
Reporter: James E. King, III

 The TThreadedServer and TThreadPoolServer do not impose limits on the number 
 of simultaneous connections, which is not useful in production as bad clients 
 can drive a server to consume too many file descriptors or have too many 
 threads.
 1. Add a barrier to TServerTransport that will be checked before accept().
 2. In the onClientConnected override (see THRIFT-3083) if the server reaches 
 the limit of the number of accepted clients, enable the barrier.
 3. In the onClientDisconnected override if the count of connected clients 
 falls below the maximum concurrent limit, clear the barrier.  This will allow 
 the limit to be changed dynamically at runtime (lowered) with drain off 
 clients until more can be accepted.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Updated] (THRIFT-3084) C++ add concurrent client limit to threaded servers

[James E. King, III (JIRA)]

 [ 
https://issues.apache.org/jira/browse/THRIFT-3084?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

James E. King, III updated THRIFT-3084:
---
Description: 
The TThreadedServer and TThreadPoolServer do not impose limits on the number of 
simultaneous connections, which is not useful in production as bad clients can 
drive a server to consume too many file descriptors or have too many threads.

1. Add a barrier to TServerTransport that will be checked before accept().

2. In the onClientConnected override (see THRIFT-3083) if the server reaches 
the limit of the number of accepted clients, enable the barrier.

3. In the onClientDisconnected override if the count of connected clients falls 
below the maximum concurrent limit, clear the barrier.  This will allow the 
limit to be changed dynamically at runtime (lowered) with drain off clients 
until more can be accepted.

Alternate proposal: Implement a Semaphore and have the servers block the 
serve() thread if the client that arrived puts the server at the concurrent 
client limit.

  was:
The TThreadedServer and TThreadPoolServer do not impose limits on the number of 
simultaneous connections, which is not useful in production as bad clients can 
drive a server to consume too many file descriptors or have too many threads.

1. Add a barrier to TServerTransport that will be checked before accept().

2. In the onClientConnected override (see THRIFT-3083) if the server reaches 
the limit of the number of accepted clients, enable the barrier.

3. In the onClientDisconnected override if the count of connected clients falls 
below the maximum concurrent limit, clear the barrier.  This will allow the 
limit to be changed dynamically at runtime (lowered) with drain off clients 
until more can be accepted.


 C++ add concurrent client limit to threaded servers
 ---

 Key: THRIFT-3084
 URL: https://issues.apache.org/jira/browse/THRIFT-3084
 Project: Thrift
  Issue Type: Improvement
  Components: C++ - Library
Affects Versions: 0.8, 0.9, 0.9.1, 0.9.2
Reporter: James E. King, III

 The TThreadedServer and TThreadPoolServer do not impose limits on the number 
 of simultaneous connections, which is not useful in production as bad clients 
 can drive a server to consume too many file descriptors or have too many 
 threads.
 1. Add a barrier to TServerTransport that will be checked before accept().
 2. In the onClientConnected override (see THRIFT-3083) if the server reaches 
 the limit of the number of accepted clients, enable the barrier.
 3. In the onClientDisconnected override if the count of connected clients 
 falls below the maximum concurrent limit, clear the barrier.  This will allow 
 the limit to be changed dynamically at runtime (lowered) with drain off 
 clients until more can be accepted.
 Alternate proposal: Implement a Semaphore and have the servers block the 
 serve() thread if the client that arrived puts the server at the concurrent 
 client limit.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Created] (THRIFT-3090) cmake build is broken

[Marco Molteni (JIRA)]

Marco Molteni created THRIFT-3090:
-

 Summary: cmake build is broken
 Key: THRIFT-3090
 URL: https://issues.apache.org/jira/browse/THRIFT-3090
 Project: Thrift
  Issue Type: Bug
Reporter: Marco Molteni


A current version of apache/thrift on github as of 2015-04-10 doesn't build 
with cmake due to multiple errors:
- some C++ targets fail to link with missing symbols, because they do not link 
against the `thrift` library
- the c_glib test targets fail to build because the reference to `shared_ptr` 
is considered ambiguous by the compiler (it resolves to both boost and stdlib 
shared_ptr)

This makes me wonder about the overall status of cmake support.

In any case, I am opening this issue in order to create a github pull request 
with a fix.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (THRIFT-3090) cmake build is broken

[James E. King, III (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-3090?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14490402#comment-14490402
 ] 

James E. King, III commented on THRIFT-3090:


Unless it was something added today, I've been able to build and test C++ and 
c_glib on Ubuntu 12.04 with gcc-4.63 and boost-1.53 using a pull from yesterday.

 cmake build is broken
 -

 Key: THRIFT-3090
 URL: https://issues.apache.org/jira/browse/THRIFT-3090
 Project: Thrift
  Issue Type: Bug
Reporter: Marco Molteni

 A current version of apache/thrift on github as of 2015-04-10 doesn't build 
 with cmake due to multiple errors:
 - some C++ targets fail to link with missing symbols, because they do not 
 link against the `thrift` library
 - the c_glib test targets fail to build because the reference to `shared_ptr` 
 is considered ambiguous by the compiler (it resolves to both boost and stdlib 
 shared_ptr)
 This makes me wonder about the overall status of cmake support.
 In any case, I am opening this issue in order to create a github pull request 
 with a fix.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[GitHub] thrift pull request: Fix THRIFT-3090 (cmake build is broken)

[marco-m]

GitHub user marco-m opened a pull request:

https://github.com/apache/thrift/pull/434

Fix THRIFT-3090 (cmake build is broken)

Problem found and fixed on Mac OS X 10.10.3, but at least the link problems 
must happen on every OS.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/marco-m/thrift THRIFT-3090

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/thrift/pull/434.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 #434


commit 73821578c4f54c36331d522b4cd685af7d539e18
Author: Marco Molteni marco.molt...@laposte.net
Date:   2015-04-10T21:51:19Z

Fix THRIFT-3090 (cmake build is broken)




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[jira] [Commented] (THRIFT-3090) cmake build is broken

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-3090?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14490434#comment-14490434
 ] 

ASF GitHub Bot commented on THRIFT-3090:


GitHub user marco-m opened a pull request:

https://github.com/apache/thrift/pull/434

Fix THRIFT-3090 (cmake build is broken)

Problem found and fixed on Mac OS X 10.10.3, but at least the link problems 
must happen on every OS.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/marco-m/thrift THRIFT-3090

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/thrift/pull/434.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 #434


commit 73821578c4f54c36331d522b4cd685af7d539e18
Author: Marco Molteni marco.molt...@laposte.net
Date:   2015-04-10T21:51:19Z

Fix THRIFT-3090 (cmake build is broken)




 cmake build is broken
 -

 Key: THRIFT-3090
 URL: https://issues.apache.org/jira/browse/THRIFT-3090
 Project: Thrift
  Issue Type: Bug
Reporter: Marco Molteni

 A current version of apache/thrift on github as of 2015-04-10 doesn't build 
 with cmake due to multiple errors:
 - some C++ targets fail to link with missing symbols, because they do not 
 link against the `thrift` library
 - the c_glib test targets fail to build because the reference to `shared_ptr` 
 is considered ambiguous by the compiler (it resolves to both boost and stdlib 
 shared_ptr)
 This makes me wonder about the overall status of cmake support.
 In any case, I am opening this issue in order to create a github pull request 
 with a fix.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (THRIFT-3090) cmake build is broken

[James E. King, III (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-3090?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14490442#comment-14490442
 ] 

James E. King, III commented on THRIFT-3090:


So the issue is with clang then?

 cmake build is broken
 -

 Key: THRIFT-3090
 URL: https://issues.apache.org/jira/browse/THRIFT-3090
 Project: Thrift
  Issue Type: Bug
Reporter: Marco Molteni

 A current version of apache/thrift on github as of 2015-04-10 doesn't build 
 with cmake due to multiple errors:
 - some C++ targets fail to link with missing symbols, because they do not 
 link against the `thrift` library
 - the c_glib test targets fail to build because the reference to `shared_ptr` 
 is considered ambiguous by the compiler (it resolves to both boost and stdlib 
 shared_ptr)
 This makes me wonder about the overall status of cmake support.
 In any case, I am opening this issue in order to create a github pull request 
 with a fix.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Comment Edited] (THRIFT-3090) cmake build is broken

[James E. King, III (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-3090?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14490442#comment-14490442
 ] 

James E. King, III edited comment on THRIFT-3090 at 4/10/15 10:04 PM:
--

So the issue is with clang then?  Recommend filling out the {{Environment}} 
section of the defect and put the OS, compiler, boost revision at a minimum in 
there.


was (Author: jking3):
So the issue is with clang then?

 cmake build is broken
 -

 Key: THRIFT-3090
 URL: https://issues.apache.org/jira/browse/THRIFT-3090
 Project: Thrift
  Issue Type: Bug
Reporter: Marco Molteni

 A current version of apache/thrift on github as of 2015-04-10 doesn't build 
 with cmake due to multiple errors:
 - some C++ targets fail to link with missing symbols, because they do not 
 link against the `thrift` library
 - the c_glib test targets fail to build because the reference to `shared_ptr` 
 is considered ambiguous by the compiler (it resolves to both boost and stdlib 
 shared_ptr)
 This makes me wonder about the overall status of cmake support.
 In any case, I am opening this issue in order to create a github pull request 
 with a fix.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[GitHub] thrift pull request: THRIFT-3076 Compatibility with Haxe 3.2.0

[Jens-G]

GitHub user Jens-G opened a pull request:

https://github.com/apache/thrift/pull/435

THRIFT-3076 Compatibility with Haxe 3.2.0

Client: Haxe
Patch: Jens Geyer

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/Jens-G/thrift THRIFT-3076-Haxe320

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/thrift/pull/435.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 #435


commit fe43fcc3329e300fe7e5efcdbdbf55fa2105c940
Author: Jens Geyer je...@apache.org
Date:   2015-04-02T22:44:27Z

THRIFT-3076 Compatibility with Haxe 3.2.0
Client: Haxe
Patch: Jens Geyer




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[jira] [Commented] (THRIFT-3076) Compatibility with Haxe 3.2.0

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-3076?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14490567#comment-14490567
 ] 

ASF GitHub Bot commented on THRIFT-3076:


GitHub user Jens-G opened a pull request:

https://github.com/apache/thrift/pull/435

THRIFT-3076 Compatibility with Haxe 3.2.0

Client: Haxe
Patch: Jens Geyer

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/Jens-G/thrift THRIFT-3076-Haxe320

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/thrift/pull/435.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 #435


commit fe43fcc3329e300fe7e5efcdbdbf55fa2105c940
Author: Jens Geyer je...@apache.org
Date:   2015-04-02T22:44:27Z

THRIFT-3076 Compatibility with Haxe 3.2.0
Client: Haxe
Patch: Jens Geyer




 Compatibility with Haxe 3.2.0
 -

 Key: THRIFT-3076
 URL: https://issues.apache.org/jira/browse/THRIFT-3076
 Project: Thrift
  Issue Type: Improvement
  Components: Haxe - Compiler, Haxe - Library
Reporter: Jens Geyer
Assignee: Jens Geyer
 Fix For: 0.9.3


 There are minor issues with the upcoming release of Haxe 3.2.0, mostly around 
 the slightly modified Int64 support.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[GitHub] thrift pull request: THRIFT-3076 Compatibility with Haxe 3.2.0

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/thrift/pull/435


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[jira] [Commented] (THRIFT-3076) Compatibility with Haxe 3.2.0

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-3076?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14490572#comment-14490572
 ] 

ASF GitHub Bot commented on THRIFT-3076:


Github user asfgit closed the pull request at:

https://github.com/apache/thrift/pull/435


 Compatibility with Haxe 3.2.0
 -

 Key: THRIFT-3076
 URL: https://issues.apache.org/jira/browse/THRIFT-3076
 Project: Thrift
  Issue Type: Improvement
  Components: Haxe - Compiler, Haxe - Library
Reporter: Jens Geyer
Assignee: Jens Geyer
 Fix For: 0.9.3


 There are minor issues with the upcoming release of Haxe 3.2.0, mostly around 
 the slightly modified Int64 support.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Resolved] (THRIFT-3076) Compatibility with Haxe 3.2.0

[Jens Geyer (JIRA)]

 [ 
https://issues.apache.org/jira/browse/THRIFT-3076?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Jens Geyer resolved THRIFT-3076.

Resolution: Fixed

Committed.

 Compatibility with Haxe 3.2.0
 -

 Key: THRIFT-3076
 URL: https://issues.apache.org/jira/browse/THRIFT-3076
 Project: Thrift
  Issue Type: Improvement
  Components: Haxe - Compiler, Haxe - Library
Reporter: Jens Geyer
Assignee: Jens Geyer
 Fix For: 0.9.3


 There are minor issues with the upcoming release of Haxe 3.2.0, mostly around 
 the slightly modified Int64 support.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (THRIFT-2441) Cannot shutdown TThreadedServer when clients are still connected

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-2441?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14490578#comment-14490578
 ] 

ASF GitHub Bot commented on THRIFT-2441:


Github user ben-craig commented on a diff in the pull request:

https://github.com/apache/thrift/pull/424#discussion_r28189195
  
--- Diff: lib/cpp/src/thrift/server/TSimpleServer.cpp ---
@@ -88,7 +88,7 @@ void TSimpleServer::serve() {
   string errStr = string(Some kind of accept exception: ) + 
tx.what();
   GlobalOutput(errStr.c_str());
   continue;
-} catch (string s) {
+} catch (const string s) {
--- End diff --

I don't know of any code in thrift that intentionally throws a string.  
Throwing a string (or anything without a vtable really) is certainly a poor 
practice.


 Cannot shutdown TThreadedServer when clients are still connected
 

 Key: THRIFT-2441
 URL: https://issues.apache.org/jira/browse/THRIFT-2441
 Project: Thrift
  Issue Type: Bug
  Components: C++ - Library
Affects Versions: 0.9.1
Reporter: Chris Stylianou
Assignee: Ben Craig
 Attachments: THRIFT-2441-prelim.patch


 When calling stop() on the TThreadedServer no interrupts are sent to the 
 client threads. This means the stop() call blocks on tasksMonitor.wait() 
 until all client naturally disconnect.
 How can we tell the client thread connections to close/exit during the 
 TThreadedServer::stop() call?



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[GitHub] thrift pull request: THRIFT-2441 prevent client connections from d...

[ben-craig]

Github user ben-craig commented on a diff in the pull request:

https://github.com/apache/thrift/pull/424#discussion_r28189195
  
--- Diff: lib/cpp/src/thrift/server/TSimpleServer.cpp ---
@@ -88,7 +88,7 @@ void TSimpleServer::serve() {
   string errStr = string(Some kind of accept exception: ) + 
tx.what();
   GlobalOutput(errStr.c_str());
   continue;
-} catch (string s) {
+} catch (const string s) {
--- End diff --

I don't know of any code in thrift that intentionally throws a string.  
Throwing a string (or anything without a vtable really) is certainly a poor 
practice.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[jira] [Commented] (THRIFT-2441) Cannot shutdown TThreadedServer when clients are still connected

[Ben Craig (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-2441?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14490579#comment-14490579
 ] 

Ben Craig commented on THRIFT-2441:
---

+1

 Cannot shutdown TThreadedServer when clients are still connected
 

 Key: THRIFT-2441
 URL: https://issues.apache.org/jira/browse/THRIFT-2441
 Project: Thrift
  Issue Type: Bug
  Components: C++ - Library
Affects Versions: 0.9.1
Reporter: Chris Stylianou
Assignee: Ben Craig
 Attachments: THRIFT-2441-prelim.patch


 When calling stop() on the TThreadedServer no interrupts are sent to the 
 client threads. This means the stop() call blocks on tasksMonitor.wait() 
 until all client naturally disconnect.
 How can we tell the client thread connections to close/exit during the 
 TThreadedServer::stop() call?



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[GitHub] thrift pull request: THRIFT-3081 consolidate client processing loo...

[ben-craig]

Github user ben-craig commented on a diff in the pull request:

https://github.com/apache/thrift/pull/433#discussion_r28189373
  
--- Diff: lib/cpp/src/thrift/server/TThreadPoolServer.cpp ---
@@ -146,9 +73,12 @@ void TThreadPoolServer::serve() {
   shared_ptrTProcessor processor = getProcessor(inputProtocol, 
outputProtocol, client);
 
   // Add to threadmanager pool
-  shared_ptrTThreadPoolServer::Task task(
-  new TThreadPoolServer::Task(*this, processor, inputProtocol, 
outputProtocol, client));
-  threadManager_-add(task, timeout_, taskExpiration_);
+  shared_ptrTConnectedClient pClient(
--- End diff --

I think you are referring to shared_from_this.  Here are some of the boost 
docs talking about the difference:

http://www.boost.org/doc/libs/1_57_0/libs/smart_ptr/enable_shared_from_this.html
http://www.boost.org/doc/libs/1_57_0/libs/smart_ptr/make_shared.html




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[jira] [Commented] (THRIFT-3081) C++ Consolidate client processing loops in TServers

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-3081?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14490584#comment-14490584
 ] 

ASF GitHub Bot commented on THRIFT-3081:


Github user ben-craig commented on a diff in the pull request:

https://github.com/apache/thrift/pull/433#discussion_r28189373
  
--- Diff: lib/cpp/src/thrift/server/TThreadPoolServer.cpp ---
@@ -146,9 +73,12 @@ void TThreadPoolServer::serve() {
   shared_ptrTProcessor processor = getProcessor(inputProtocol, 
outputProtocol, client);
 
   // Add to threadmanager pool
-  shared_ptrTThreadPoolServer::Task task(
-  new TThreadPoolServer::Task(*this, processor, inputProtocol, 
outputProtocol, client));
-  threadManager_-add(task, timeout_, taskExpiration_);
+  shared_ptrTConnectedClient pClient(
--- End diff --

I think you are referring to shared_from_this.  Here are some of the boost 
docs talking about the difference:

http://www.boost.org/doc/libs/1_57_0/libs/smart_ptr/enable_shared_from_this.html
http://www.boost.org/doc/libs/1_57_0/libs/smart_ptr/make_shared.html




 C++ Consolidate client processing loops in TServers
 ---

 Key: THRIFT-3081
 URL: https://issues.apache.org/jira/browse/THRIFT-3081
 Project: Thrift
  Issue Type: Improvement
  Components: C++ - Library
Affects Versions: 0.8, 0.9, 0.9.1, 0.9.2
Reporter: James E. King, III

 Currently each of TSimpleServer, TThreadedServer, and TThreadPoolServer have 
 their own very similar but not quite identical way of processing a client's 
 lifetime.  The code has been copied around and changed; for example a 
 TThreadPoolServer handles TTransportExceptions from process() differently 
 than a TThreadedServer does.
 There are certain requirements for this processing loop that needs to be met 
 by every client.  Consolidating these three disparate implementations of the 
 client processing loop into one will provide consistency as well as easier 
 maintenance, as there will be one common client processing loop that will 
 contain all the logic from {{eventHandler-createContext}} through 
 {{client-close}}.
 It was also discovered that all three implementations call peek() in each 
 loop which causes more recv calls than are really needed.  Will experiment 
 with removing peek entirely; expectation is that it is sufficient to have 
 exception handling around process() and/or have process() return false to end 
 the processing loop, and peek() is likely an unnecessary temporary band-aid 
 that got left there.
 This was inspired by changes in THRIFT-2441 and I was encouraged to make this 
 a separate body of work from that change so that it can be reviewed in 
 isolation from other changes.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[GitHub] thrift pull request: THRIFT-3081 consolidate client processing loo...

[ben-craig]

Github user ben-craig commented on a diff in the pull request:

https://github.com/apache/thrift/pull/433#discussion_r28189526
  
--- Diff: lib/cpp/src/thrift/server/TConnectedClient.cpp ---
@@ -0,0 +1,105 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * License); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * AS IS BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include thrift/server/TConnectedClient.h
+
+namespace apache { namespace thrift { namespace server {
+
+using apache::thrift::TProcessor;
+using apache::thrift::protocol::TProtocol;
+using apache::thrift::server::TServerEventHandler;
+using apache::thrift::transport::TTransport;
+using apache::thrift::transport::TTransportException;
+using boost::shared_ptr;
+using std::string;
+
+TConnectedClient::TConnectedClient(const string serverType,
+   const shared_ptrTProcessor processor,
+   const shared_ptrTProtocol 
inputProtocol,
+   const shared_ptrTProtocol 
outputProtocol,
+   const shared_ptrTServerEventHandler 
eventHandler,
+   const shared_ptrTTransport client)
+
+  : serverType_(serverType),
+processor_(processor),
+inputProtocol_(inputProtocol),
+outputProtocol_(outputProtocol),
+eventHandler_(eventHandler),
+client_(client),
+opaqueContext_(0)
+{}
+
+TConnectedClient::~TConnectedClient()
+{}
+
+void TConnectedClient::run()
+{
+  if (eventHandler_) {
+opaqueContext_ = eventHandler_-createContext(inputProtocol_, 
outputProtocol_);
+  }
+
+  for (;;) {
+if (eventHandler_) {
+  eventHandler_-processContext(opaqueContext_, client_);
+}
+
+try {
+  if (!processor_-process(inputProtocol_, outputProtocol_, 
opaqueContext_)) {
+break;
+  }
+} catch (const TTransportException ttx) {
+  if (ttx.getType() != TTransportException::END_OF_FILE 
+  ttx.getType() != TTransportException::INTERRUPTED) {
+string errStr = (serverType_ +  client died: ) + ttx.what();
+GlobalOutput(errStr.c_str());
+  }
+  break;
+}
+  }
--- End diff --

On the client side of things, I agree with catching very little.

For the server side, my preferred policy would be to catch just about 
everything, log as best we can, then close the connection to the client that 
caused the exception.  

This code is run pretty close to threadMain, so any uncaught exceptions 
will bring down the entire process, resulting in a DoS for other clients.  In 
addition, some exceptions are sort-of expected, like std::bad_alloc.  If you 
don't catch bad_alloc, then the client that puts your server one over capacity 
will make the server as a whole go away.




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[jira] [Commented] (THRIFT-3081) C++ Consolidate client processing loops in TServers

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-3081?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14490593#comment-14490593
 ] 

ASF GitHub Bot commented on THRIFT-3081:


Github user ben-craig commented on a diff in the pull request:

https://github.com/apache/thrift/pull/433#discussion_r28189526
  
--- Diff: lib/cpp/src/thrift/server/TConnectedClient.cpp ---
@@ -0,0 +1,105 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * License); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * AS IS BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include thrift/server/TConnectedClient.h
+
+namespace apache { namespace thrift { namespace server {
+
+using apache::thrift::TProcessor;
+using apache::thrift::protocol::TProtocol;
+using apache::thrift::server::TServerEventHandler;
+using apache::thrift::transport::TTransport;
+using apache::thrift::transport::TTransportException;
+using boost::shared_ptr;
+using std::string;
+
+TConnectedClient::TConnectedClient(const string serverType,
+   const shared_ptrTProcessor processor,
+   const shared_ptrTProtocol 
inputProtocol,
+   const shared_ptrTProtocol 
outputProtocol,
+   const shared_ptrTServerEventHandler 
eventHandler,
+   const shared_ptrTTransport client)
+
+  : serverType_(serverType),
+processor_(processor),
+inputProtocol_(inputProtocol),
+outputProtocol_(outputProtocol),
+eventHandler_(eventHandler),
+client_(client),
+opaqueContext_(0)
+{}
+
+TConnectedClient::~TConnectedClient()
+{}
+
+void TConnectedClient::run()
+{
+  if (eventHandler_) {
+opaqueContext_ = eventHandler_-createContext(inputProtocol_, 
outputProtocol_);
+  }
+
+  for (;;) {
+if (eventHandler_) {
+  eventHandler_-processContext(opaqueContext_, client_);
+}
+
+try {
+  if (!processor_-process(inputProtocol_, outputProtocol_, 
opaqueContext_)) {
+break;
+  }
+} catch (const TTransportException ttx) {
+  if (ttx.getType() != TTransportException::END_OF_FILE 
+  ttx.getType() != TTransportException::INTERRUPTED) {
+string errStr = (serverType_ +  client died: ) + ttx.what();
+GlobalOutput(errStr.c_str());
+  }
+  break;
+}
+  }
--- End diff --

On the client side of things, I agree with catching very little.

For the server side, my preferred policy would be to catch just about 
everything, log as best we can, then close the connection to the client that 
caused the exception.  

This code is run pretty close to threadMain, so any uncaught exceptions 
will bring down the entire process, resulting in a DoS for other clients.  In 
addition, some exceptions are sort-of expected, like std::bad_alloc.  If you 
don't catch bad_alloc, then the client that puts your server one over capacity 
will make the server as a whole go away.




 C++ Consolidate client processing loops in TServers
 ---

 Key: THRIFT-3081
 URL: https://issues.apache.org/jira/browse/THRIFT-3081
 Project: Thrift
  Issue Type: Improvement
  Components: C++ - Library
Affects Versions: 0.8, 0.9, 0.9.1, 0.9.2
Reporter: James E. King, III

 Currently each of TSimpleServer, TThreadedServer, and TThreadPoolServer have 
 their own very similar but not quite identical way of processing a client's 
 lifetime.  The code has been copied around and changed; for example a 
 TThreadPoolServer handles TTransportExceptions from process() differently 
 than a TThreadedServer does.
 There are certain requirements for this processing loop that needs to be met 
 by every client.  Consolidating these three disparate implementations of the 
 client processing loop into one will 

[jira] [Commented] (THRIFT-3081) C++ Consolidate client processing loops in TServers

[Ben Craig (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-3081?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14490598#comment-14490598
 ] 

Ben Craig commented on THRIFT-3081:
---

I approve of the general direction of the recently closed pull request.

 C++ Consolidate client processing loops in TServers
 ---

 Key: THRIFT-3081
 URL: https://issues.apache.org/jira/browse/THRIFT-3081
 Project: Thrift
  Issue Type: Improvement
  Components: C++ - Library
Affects Versions: 0.8, 0.9, 0.9.1, 0.9.2
Reporter: James E. King, III

 Currently each of TSimpleServer, TThreadedServer, and TThreadPoolServer have 
 their own very similar but not quite identical way of processing a client's 
 lifetime.  The code has been copied around and changed; for example a 
 TThreadPoolServer handles TTransportExceptions from process() differently 
 than a TThreadedServer does.
 There are certain requirements for this processing loop that needs to be met 
 by every client.  Consolidating these three disparate implementations of the 
 client processing loop into one will provide consistency as well as easier 
 maintenance, as there will be one common client processing loop that will 
 contain all the logic from {{eventHandler-createContext}} through 
 {{client-close}}.
 It was also discovered that all three implementations call peek() in each 
 loop which causes more recv calls than are really needed.  Will experiment 
 with removing peek entirely; expectation is that it is sufficient to have 
 exception handling around process() and/or have process() return false to end 
 the processing loop, and peek() is likely an unnecessary temporary band-aid 
 that got left there.
 This was inspired by changes in THRIFT-2441 and I was encouraged to make this 
 a separate body of work from that change so that it can be reviewed in 
 isolation from other changes.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (THRIFT-3076) Compatibility with Haxe 3.2.0

[Hudson (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-3076?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14490609#comment-14490609
 ] 

Hudson commented on THRIFT-3076:


SUCCESS: Integrated in Thrift #1498 (See 
[https://builds.apache.org/job/Thrift/1498/])
THRIFT-3076 Compatibility with Haxe 3.2.0 (jensg: rev 
86f7350f90c7432c9415cb43d003ff7e6385c258)
* lib/haxe/src/org/apache/thrift/helper/Int64Map.hx
* lib/haxe/src/org/apache/thrift/helper/BitConverter.hx
* compiler/cpp/src/generate/t_haxe_generator.cc
* lib/haxe/src/org/apache/thrift/protocol/TBinaryProtocol.hx
* lib/haxe/src/org/apache/thrift/transport/TTransportException.hx
* lib/haxe/src/org/apache/thrift/protocol/TCompactProtocol.hx
* lib/haxe/src/org/apache/thrift/TApplicationException.hx
* lib/haxe/src/org/apache/thrift/protocol/TProtocolException.hx
* lib/haxe/src/org/apache/thrift/protocol/TMultiplexedProcessor.hx
* lib/haxe/src/org/apache/thrift/transport/TFullDuplexHttpClient.hx


 Compatibility with Haxe 3.2.0
 -

 Key: THRIFT-3076
 URL: https://issues.apache.org/jira/browse/THRIFT-3076
 Project: Thrift
  Issue Type: Improvement
  Components: Haxe - Compiler, Haxe - Library
Reporter: Jens Geyer
Assignee: Jens Geyer
 Fix For: 0.9.3


 There are minor issues with the upcoming release of Haxe 3.2.0, mostly around 
 the slightly modified Int64 support.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (THRIFT-3084) C++ add concurrent client limit to threaded servers

[Randy Abernethy (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-3084?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14490651#comment-14490651
 ] 

Randy Abernethy commented on THRIFT-3084:
-

Hey Ben: 

I agree with keeping the TThreadedServer name for 0.9.x (typedef or whatever). 
I would like to see it dropped in 1.0 though. Questions like: Should I use 
TThreaded or TThreadPool when I want a non eventlib server? and What is the 
difference between …? will go away. Given that the TThreaded interface will be 
supported in TThreadPoolServer I don't think its removal will create a 
significant burden. 

-Randy

 C++ add concurrent client limit to threaded servers
 ---

 Key: THRIFT-3084
 URL: https://issues.apache.org/jira/browse/THRIFT-3084
 Project: Thrift
  Issue Type: Improvement
  Components: C++ - Library
Affects Versions: 0.8, 0.9, 0.9.1, 0.9.2
Reporter: James E. King, III

 The TThreadedServer and TThreadPoolServer do not impose limits on the number 
 of simultaneous connections, which is not useful in production as bad clients 
 can drive a server to consume too many file descriptors or have too many 
 threads.
 1. Add a barrier to TServerTransport that will be checked before accept().
 2. In the onClientConnected override (see THRIFT-3083) if the server reaches 
 the limit of the number of accepted clients, enable the barrier.
 3. In the onClientDisconnected override if the count of connected clients 
 falls below the maximum concurrent limit, clear the barrier.  This will allow 
 the limit to be changed dynamically at runtime (lowered) with drain off 
 clients until more can be accepted.
 Alternate proposal: Implement a Semaphore and have the servers block the 
 serve() thread if the client that arrived puts the server at the concurrent 
 client limit.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (THRIFT-3081) C++ Consolidate client processing loops in TServers

[James E. King, III (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-3081?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14490707#comment-14490707
 ] 

James E. King, III commented on THRIFT-3081:


Thanks, however even on the server side how are you going to find and fix bugs 
if you lose the context in which they occurred (via core)?  I disagree with the 
notion that one would catch std::exception or ...  - instead let it core and 
let the bug be fixed so it will never core again.

Once THRIFT-2441 is merged, I have patches queued up to 3081, then 3083, and I 
am working on 3084 now.

 C++ Consolidate client processing loops in TServers
 ---

 Key: THRIFT-3081
 URL: https://issues.apache.org/jira/browse/THRIFT-3081
 Project: Thrift
  Issue Type: Improvement
  Components: C++ - Library
Affects Versions: 0.8, 0.9, 0.9.1, 0.9.2
Reporter: James E. King, III

 Currently each of TSimpleServer, TThreadedServer, and TThreadPoolServer have 
 their own very similar but not quite identical way of processing a client's 
 lifetime.  The code has been copied around and changed; for example a 
 TThreadPoolServer handles TTransportExceptions from process() differently 
 than a TThreadedServer does.
 There are certain requirements for this processing loop that needs to be met 
 by every client.  Consolidating these three disparate implementations of the 
 client processing loop into one will provide consistency as well as easier 
 maintenance, as there will be one common client processing loop that will 
 contain all the logic from {{eventHandler-createContext}} through 
 {{client-close}}.
 It was also discovered that all three implementations call peek() in each 
 loop which causes more recv calls than are really needed.  Will experiment 
 with removing peek entirely; expectation is that it is sufficient to have 
 exception handling around process() and/or have process() return false to end 
 the processing loop, and peek() is likely an unnecessary temporary band-aid 
 that got left there.
 This was inspired by changes in THRIFT-2441 and I was encouraged to make this 
 a separate body of work from that change so that it can be reviewed in 
 isolation from other changes.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (THRIFT-3084) C++ add concurrent client limit to threaded servers

[James E. King, III (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-3084?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14490710#comment-14490710
 ] 

James E. King, III commented on THRIFT-3084:


The proper way to do this would be to deprecate TThreadedServer in 0.9.x with a 
comment that says it disappears in 1.0.  I am okay with that as well.  The 
default ThreadFactory is essentially how TThtreadedServer works anyway; however 
I will be adding concurrent client limits via Semaphore in THRIFT-3084 so the 
standard servers should have everything they need to be production quality and 
resource bound.

 C++ add concurrent client limit to threaded servers
 ---

 Key: THRIFT-3084
 URL: https://issues.apache.org/jira/browse/THRIFT-3084
 Project: Thrift
  Issue Type: Improvement
  Components: C++ - Library
Affects Versions: 0.8, 0.9, 0.9.1, 0.9.2
Reporter: James E. King, III

 The TThreadedServer and TThreadPoolServer do not impose limits on the number 
 of simultaneous connections, which is not useful in production as bad clients 
 can drive a server to consume too many file descriptors or have too many 
 threads.
 1. Add a barrier to TServerTransport that will be checked before accept().
 2. In the onClientConnected override (see THRIFT-3083) if the server reaches 
 the limit of the number of accepted clients, enable the barrier.
 3. In the onClientDisconnected override if the count of connected clients 
 falls below the maximum concurrent limit, clear the barrier.  This will allow 
 the limit to be changed dynamically at runtime (lowered) with drain off 
 clients until more can be accepted.
 Alternate proposal: Implement a Semaphore and have the servers block the 
 serve() thread if the client that arrived puts the server at the concurrent 
 client limit.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (THRIFT-3084) C++ add concurrent client limit to threaded servers

[Randy Abernethy (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-3084?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14490722#comment-14490722
 ] 

Randy Abernethy commented on THRIFT-3084:
-

Hey James:

I have a different view on the implementation inheritance front. 

TSimpleServer has a responsibility, be simple, be a good way to understand 
thrift server basics and provide a clean, simple, single threaded 
implementation. TThreadPoolServer has a very different purpose, provide a 
thread per client based server implementation which can be used for handling 
scaled production loads. Semantically there is no implied implementation bond 
here, only the fact that they are both Apache Thrift servers.

TSimpleServer should have the TServer interface, but should it have the same 
implementation as TThreadPoolServer? It mostly does now but will it always? Is 
it easier or harder for me to understand the simple server with code in various 
classes handling the implementation? What if someone decides to add an 
IOCompletion port server for Windows? Should it use the framework 
implementation designed for TThreadPoolServer? Can the NonblockingServer? What 
if I need to update the TServerFramework implementation for TThreadPoolServer? 
I would need to be an expert in all of the derived servers to know for sure 
that my implementation changes will work with each of them. At this point 
TSimpleServer is not very simple, it is no longer performing its one 
responsibility. 

Tightly coupled classes (and systems in general) make maintenance hard. There 
is no tighter way to couple classes than implementation inheritance. There are 
some fairly well thought out guidelines that relate to this issue, for example:
Alexandrescu, Andrei; Herb Sutter (2004-10-25). C++ Coding Standards: 101 
Rules, Guidelines, and Best Practices 
- 34. Prefer composition to inheritance 
- 36. Prefer providing abstract interfaces 
- 37. Public inheritance is substitutability. Inherit, not to reuse, but to be 
reused

I have seen many crimes against encapsulation, separation of concerns and KIS 
committed in the name of DRY.

I am all for the other code improvements, particularly the TThreadPoolServer 
code you have written in the TServerFramework class. My “keep it simple” 
approach would be to integrate the TThreadPoolServer fixes standalone, 
deprecate TThreadedServer (implementing it with TThreadPoolServer as Ben 
suggests) and keep TSimpleServer simple and self-contained.

Just my 2 cents and certainly subjective. You are making things better patch by 
patch so I am a fan either way.

Best,
Randy


 C++ add concurrent client limit to threaded servers
 ---

 Key: THRIFT-3084
 URL: https://issues.apache.org/jira/browse/THRIFT-3084
 Project: Thrift
  Issue Type: Improvement
  Components: C++ - Library
Affects Versions: 0.8, 0.9, 0.9.1, 0.9.2
Reporter: James E. King, III

 The TThreadedServer and TThreadPoolServer do not impose limits on the number 
 of simultaneous connections, which is not useful in production as bad clients 
 can drive a server to consume too many file descriptors or have too many 
 threads.
 1. Add a barrier to TServerTransport that will be checked before accept().
 2. In the onClientConnected override (see THRIFT-3083) if the server reaches 
 the limit of the number of accepted clients, enable the barrier.
 3. In the onClientDisconnected override if the count of connected clients 
 falls below the maximum concurrent limit, clear the barrier.  This will allow 
 the limit to be changed dynamically at runtime (lowered) with drain off 
 clients until more can be accepted.
 Alternate proposal: Implement a Semaphore and have the servers block the 
 serve() thread if the client that arrived puts the server at the concurrent 
 client limit.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)