[
https://issues.apache.org/jira/browse/THRIFT-3084?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=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 subclass code
duplication and the merits of such. I would argue the changes I have made
simplify maintenance because now there is only one place in the code one needs
to change the common client processing loop (THRIFT-3081) or the common server
processing loop (THRIFT-3083).\
> 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)
