You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by jeking3 <gi...@git.apache.org> on 2016/04/05 00:22:29 UTC
[GitHub] thrift pull request: THRIFT-3768 fix TThreadedServer refactoring i...
GitHub user jeking3 opened a pull request:
https://github.com/apache/thrift/pull/977
THRIFT-3768 fix TThreadedServer refactoring issues with client lifetime guarantees
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jeking3/thrift defect/THRIFT-3768
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/977.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 #977
----
commit 3f44af8a474591eb0879983efc35534e17a0b533
Author: Jim King <ji...@simplivity.com>
Date: 2016-04-04T22:12:49Z
THRIFT-3768 fix TThreadedServer refactoring issues with client lifetime guarantees
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3768 fix TThreadedServer refactoring i...
Posted by tpcwang <gi...@git.apache.org>.
Github user tpcwang commented on a diff in the pull request:
https://github.com/apache/thrift/pull/977#discussion_r58472724
--- Diff: lib/cpp/src/thrift/server/TThreadedServer.h ---
@@ -20,19 +20,26 @@
#ifndef _THRIFT_SERVER_TTHREADEDSERVER_H_
#define _THRIFT_SERVER_TTHREADEDSERVER_H_ 1
-#include <thrift/concurrency/Monitor.h>
#include <thrift/concurrency/PlatformThreadFactory.h>
#include <thrift/concurrency/Thread.h>
-#include <thrift/server/TServerFramework.h>
+#include <thrift/server/TThreadPoolServer.h>
namespace apache {
namespace thrift {
namespace server {
/**
- * Manage clients using a thread pool.
+ * Manage clients using threads. Once the refactoring for THRIFT-3083 took place it became
+ * obvious that the differences between the two threaded server types was becoming insignificant.
+ * Therefore to satisfy THRIFT-3096 and fix THRIFT-3768, TThreadedServer is simply a wrapper
+ * around TThreadedPoolServer now. If backwards compatibility was not a concern, it would have
+ * been removed.
+ *
+ * The default thread pool size is
--- End diff --
Unfinished comment.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3768 fix TThreadedServer refactoring i...
Posted by tpcwang <gi...@git.apache.org>.
Github user tpcwang commented on a diff in the pull request:
https://github.com/apache/thrift/pull/977#discussion_r58473050
--- Diff: lib/cpp/src/thrift/server/TThreadedServer.h ---
@@ -20,19 +20,26 @@
#ifndef _THRIFT_SERVER_TTHREADEDSERVER_H_
#define _THRIFT_SERVER_TTHREADEDSERVER_H_ 1
-#include <thrift/concurrency/Monitor.h>
#include <thrift/concurrency/PlatformThreadFactory.h>
#include <thrift/concurrency/Thread.h>
-#include <thrift/server/TServerFramework.h>
+#include <thrift/server/TThreadPoolServer.h>
namespace apache {
namespace thrift {
namespace server {
/**
- * Manage clients using a thread pool.
+ * Manage clients using threads. Once the refactoring for THRIFT-3083 took place it became
+ * obvious that the differences between the two threaded server types was becoming insignificant.
+ * Therefore to satisfy THRIFT-3096 and fix THRIFT-3768, TThreadedServer is simply a wrapper
+ * around TThreadedPoolServer now. If backwards compatibility was not a concern, it would have
+ * been removed.
+ *
+ * The default thread pool size is
*/
-class TThreadedServer : public TServerFramework {
+
+/* [[deprecated]] */
+class TThreadedServer : public TThreadPoolServer {
--- End diff --
I'd suggest using composition and the thread manager internally rather than inheriting from TThreadPoolServer seeing that TThreadPoolServer exposes other methods that would defeat the original purpose of the TThreadedServer.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3768 fix TThreadedServer refactoring i...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the pull request:
https://github.com/apache/thrift/pull/977#issuecomment-206358845
Declining this pull request - I have a far simpler one to submit.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #977: THRIFT-3768 fix TThreadedServer refactoring issues...
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on a diff in the pull request:
https://github.com/apache/thrift/pull/977#discussion_r67414520
--- Diff: lib/cpp/src/thrift/server/TThreadedServer.h ---
@@ -20,19 +20,26 @@
#ifndef _THRIFT_SERVER_TTHREADEDSERVER_H_
#define _THRIFT_SERVER_TTHREADEDSERVER_H_ 1
-#include <thrift/concurrency/Monitor.h>
#include <thrift/concurrency/PlatformThreadFactory.h>
#include <thrift/concurrency/Thread.h>
-#include <thrift/server/TServerFramework.h>
+#include <thrift/server/TThreadPoolServer.h>
namespace apache {
namespace thrift {
namespace server {
/**
- * Manage clients using a thread pool.
+ * Manage clients using threads. Once the refactoring for THRIFT-3083 took place it became
+ * obvious that the differences between the two threaded server types was becoming insignificant.
+ * Therefore to satisfy THRIFT-3096 and fix THRIFT-3768, TThreadedServer is simply a wrapper
+ * around TThreadedPoolServer now. If backwards compatibility was not a concern, it would have
+ * been removed.
+ *
+ * The default thread pool size is
--- End diff --
Still the case.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3768 fix TThreadedServer refactoring i...
Posted by tpcwang <gi...@git.apache.org>.
Github user tpcwang commented on a diff in the pull request:
https://github.com/apache/thrift/pull/977#discussion_r58473198
--- Diff: lib/cpp/src/thrift/server/TThreadedServer.cpp ---
@@ -77,44 +83,29 @@ TThreadedServer::TThreadedServer(const shared_ptr<TProcessor>& processor,
const shared_ptr<TProtocolFactory>& inputProtocolFactory,
const shared_ptr<TProtocolFactory>& outputProtocolFactory,
const shared_ptr<ThreadFactory>& threadFactory)
- : TServerFramework(processor,
- serverTransport,
- inputTransportFactory,
- outputTransportFactory,
- inputProtocolFactory,
- outputProtocolFactory),
- threadFactory_(threadFactory) {
+ : TThreadPoolServer(processor,
+ serverTransport,
+ inputTransportFactory,
+ outputTransportFactory,
+ inputProtocolFactory,
+ outputProtocolFactory,
+ apache::thrift::concurrency::ThreadManager::newSimpleThreadManager(0, 0)) {
+ threadManager_->threadFactory(threadFactory);
+ threadManager_->start();
}
TThreadedServer::~TThreadedServer() {
}
-void TThreadedServer::serve() {
- TServerFramework::serve();
-
- // Drain all clients - no more will arrive
- try {
- Synchronized s(clientsMonitor_);
- while (getConcurrentClientCount() > 0) {
- clientsMonitor_.wait();
- }
- } catch (TException& tx) {
- string errStr = string("TThreadedServer: Exception joining workers: ") + tx.what();
- GlobalOutput(errStr.c_str());
+void TThreadedServer::onClientConnected(const shared_ptr<TConnectedClient>& pClient) {
+ if (!threadManager_->idleWorkerCount())
--- End diff --
I would personally like to see how the threads would be reaped before we go down this path, but that's mostly because I am unfamiliar with the thread manager. I'll defer to you and the maintainers on whether to do this without the accompanying thread manager change.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3768 fix TThreadedServer refactoring i...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/977#discussion_r58481087
--- Diff: lib/cpp/src/thrift/server/TThreadedServer.cpp ---
@@ -77,44 +83,29 @@ TThreadedServer::TThreadedServer(const shared_ptr<TProcessor>& processor,
const shared_ptr<TProtocolFactory>& inputProtocolFactory,
const shared_ptr<TProtocolFactory>& outputProtocolFactory,
const shared_ptr<ThreadFactory>& threadFactory)
- : TServerFramework(processor,
- serverTransport,
- inputTransportFactory,
- outputTransportFactory,
- inputProtocolFactory,
- outputProtocolFactory),
- threadFactory_(threadFactory) {
+ : TThreadPoolServer(processor,
+ serverTransport,
+ inputTransportFactory,
+ outputTransportFactory,
+ inputProtocolFactory,
+ outputProtocolFactory,
+ apache::thrift::concurrency::ThreadManager::newSimpleThreadManager(0, 0)) {
+ threadManager_->threadFactory(threadFactory);
+ threadManager_->start();
}
TThreadedServer::~TThreadedServer() {
}
-void TThreadedServer::serve() {
- TServerFramework::serve();
-
- // Drain all clients - no more will arrive
- try {
- Synchronized s(clientsMonitor_);
- while (getConcurrentClientCount() > 0) {
- clientsMonitor_.wait();
- }
- } catch (TException& tx) {
- string errStr = string("TThreadedServer: Exception joining workers: ") + tx.what();
- GlobalOutput(errStr.c_str());
+void TThreadedServer::onClientConnected(const shared_ptr<TConnectedClient>& pClient) {
+ if (!threadManager_->idleWorkerCount())
--- End diff --
As submitted the number of threads will grow to the maximum number of concurrent connections and will not shrink. Additional work is needed to allow ThreadManager::removeWorker() to do the right thing when many threads arrive at it at the same time. In gdb I found that workerCount_ was 10 but workerMaxCount_ was 0; the stop behavior for ThreadManager calls removeWorker(workerCount_) and gets an exception, which eventually causes a core. This is why, for this submission, I left it as stated.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3768 fix TThreadedServer refactoring i...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 closed the pull request at:
https://github.com/apache/thrift/pull/977
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---