You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by nsuke <gi...@git.apache.org> on 2016/05/05 08:09:29 UTC
[GitHub] thrift pull request: THRIFT-3814 Fix contention in TNonblockingSer...
GitHub user nsuke opened a pull request:
https://github.com/apache/thrift/pull/1005
THRIFT-3814 Fix contention in TNonblockingServerTest
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nsuke/thrift THRIFT-3814
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1005.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 #1005
----
commit b465de4a3f0bd89ce7e151ebc382b7f0d46af167
Author: Nobuaki Sukegawa <ns...@apache.org>
Date: 2016-05-05T07:29:13Z
THRIFT-3814 Fix contention in TNonblockingServerTest
----
---
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-3814 Fix contention in TNonblockingSer...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1005#discussion_r62283672
--- Diff: lib/cpp/test/TNonblockingServerTest.cpp ---
@@ -47,14 +47,32 @@ struct Handler : public test::ParentServiceIf {
class Fixture {
private:
struct Runner : public apache::thrift::concurrency::Runnable {
+ int port;
+ boost::shared_ptr<event_base> userEventBase;
+ boost::shared_ptr<TProcessor> processor;
boost::shared_ptr<server::TNonblockingServer> server;
- bool error;
+
virtual void run() {
- error = false;
+ // When binding to explicit port, allow retrying to workaround bind failures on ports in use
+ int retryCount = port ? 10 : 0;
+ startServer(retryCount);
+ }
+
+ private:
+ void startServer(int retry_count) {
try {
+ server.reset(new server::TNonblockingServer(processor, port));
+ if (userEventBase) {
+ server->registerEvents(userEventBase.get());
+ }
server->serve();
- } catch (const TException&) {
- error = true;
+ } catch (const transport::TTransportException&) {
+ if (retry_count > 0) {
+ ++port;
+ startServer(retry_count - 1);
+ } else {
+ throw;
--- End diff --
It's a test failure so my intention here was "no need to catch" and let it die with std::terminate with exception log.
The original `error` flag was needed for retrying from the main thread.
---
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-3814 Fix contention in TNonblockingSer...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/1005
---
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-3814 Fix contention in TNonblockingSer...
Posted by tpcwang <gi...@git.apache.org>.
Github user tpcwang commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1005#discussion_r62278341
--- Diff: lib/cpp/test/TNonblockingServerTest.cpp ---
@@ -47,14 +47,32 @@ struct Handler : public test::ParentServiceIf {
class Fixture {
private:
struct Runner : public apache::thrift::concurrency::Runnable {
+ int port;
+ boost::shared_ptr<event_base> userEventBase;
+ boost::shared_ptr<TProcessor> processor;
boost::shared_ptr<server::TNonblockingServer> server;
- bool error;
+
virtual void run() {
- error = false;
+ // When binding to explicit port, allow retrying to workaround bind failures on ports in use
+ int retryCount = port ? 10 : 0;
+ startServer(retryCount);
+ }
+
+ private:
+ void startServer(int retry_count) {
try {
+ server.reset(new server::TNonblockingServer(processor, port));
+ if (userEventBase) {
+ server->registerEvents(userEventBase.get());
+ }
server->serve();
- } catch (const TException&) {
- error = true;
+ } catch (const transport::TTransportException&) {
+ if (retry_count > 0) {
+ ++port;
+ startServer(retry_count - 1);
+ } else {
+ throw;
--- End diff --
Seems bad to throw an unhandled exception from a thread, but maybe this is catastrophic and rare enough that this is fine? The rest of the change looks good to me.
---
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.
---