[
https://issues.apache.org/jira/browse/THRIFT-3081?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=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_ptr<TProcessor>& processor,
+ const shared_ptr<TProtocol>&
inputProtocol,
+ const shared_ptr<TProtocol>&
outputProtocol,
+ const shared_ptr<TServerEventHandler>&
eventHandler,
+ const shared_ptr<TTransport>& 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 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)
