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 2015/04/04 21:34:08 UTC
[GitHub] thrift pull request: [THRIFT-2441] prevent client connections from...
GitHub user jeking3 opened a pull request:
https://github.com/apache/thrift/pull/424
[THRIFT-2441] prevent client connections from delaying server stop
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jeking3/thrift bugfix/THRIFT-2441-disconnect-clients-on-server-stop
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/424.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 #424
----
commit 4113f4f50bc4a6a1488b56db14a15e96527c44a7
Author: Jim King <ji...@simplivity.com>
Date: 2015-04-04T19:26:54Z
[THRIFT-2441] prevent client connections from delaying server stop
----
---
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-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/424#discussion_r28120482
--- Diff: lib/cpp/test/TestTServerSocket.h ---
@@ -0,0 +1,31 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#include <thrift/transport/TServerSocket.h>
+
+class TestTServerSocket : public apache::thrift::transport::TServerSocket
--- End diff --
It allows acceptImpl() to be called directly by the test.
---
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-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
GitHub user jeking3 reopened a pull request:
https://github.com/apache/thrift/pull/424
THRIFT-2441 prevent client connections from delaying server stop
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jeking3/thrift bugfix/THRIFT-2441-disconnect-clients-on-server-stop
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/424.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 #424
----
commit 4113f4f50bc4a6a1488b56db14a15e96527c44a7
Author: Jim King <ji...@simplivity.com>
Date: 2015-04-04T19:26:54Z
[THRIFT-2441] prevent client connections from delaying server stop
commit 730877b51d6ef96f84cad34e055f01fd9503aa96
Author: Jim King <ji...@simplivity.com>
Date: 2015-04-04T23:09:16Z
[THRIFT-2441] fix stop handling of simple, threaded server
commit ad68c576a75257dcca26b2c1b88fab27dcd967f0
Author: Jim King <ji...@simplivity.com>
Date: 2015-04-10T04:10:37Z
Merge branch 'master' of github.com:jeking3/thrift into bugfix/THRIFT-2441-disconnect-clients-on-server-stop
commit 251d4d4a2edb3ee95f7b200c70333d7fdc8171c8
Author: Jim King <ji...@simplivity.com>
Date: 2015-04-10T04:44:10Z
THRIFT-2441 addressed code review concerns
commit 0ca766bfd4c8be98481c7295f1bec5e040959ffc
Author: Jim King <ji...@simplivity.com>
Date: 2015-04-12T10:42:21Z
THRIFT-2441 added TServerIntegrationTest to prove combined behavior is correct; fixed TSocket handling of ECONNRESET
----
---
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-2441 prevent client connections from d...
Posted by bufferoverflow <gi...@git.apache.org>.
Github user bufferoverflow commented on a diff in the pull request:
https://github.com/apache/thrift/pull/424#discussion_r28204494
--- Diff: lib/cpp/src/thrift/transport/TSocket.cpp ---
@@ -493,29 +585,9 @@ uint32_t TSocket::read(uint8_t* buf, uint32_t len) {
goto try_again;
}
-#if defined __FreeBSD__ || defined __MACH__
if (errno_copy == THRIFT_ECONNRESET) {
- /* shigin: freebsd doesn't follow POSIX semantic of recv and fails with
- * THRIFT_ECONNRESET if peer performed shutdown
- * edhall: eliminated close() since we do that in the destructor.
- */
return 0;
}
-#endif
-
-#ifdef _WIN32
--- End diff --
This sequence is needed for some platforms, did you some tests on windows?
---
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-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the pull request:
https://github.com/apache/thrift/pull/424#issuecomment-92033000
Nevermind, it is my fault - fixing it..
---
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-2441 prevent client connections from d...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/424#discussion_r28117844
--- Diff: lib/cpp/test/TSocketInterruptTest.cpp ---
@@ -0,0 +1,147 @@
+/*
+ * 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.
+ */
+
+#define BOOST_TEST_MODULE TSocketInterruptTest
+#include <boost/test/auto_unit_test.hpp>
+
+#include <boost/bind.hpp>
+#include <boost/chrono/duration.hpp>
+#include <boost/date_time/posix_time/posix_time_duration.hpp>
+#include <boost/thread/thread.hpp>
+#include <thrift/transport/TSocket.h>
+#include <thrift/transport/TServerSocket.h>
+#include "TestPortFixture.h"
+#include "TestTServerSocket.h"
+
+using apache::thrift::transport::TServerSocket;
+using apache::thrift::transport::TSocket;
+using apache::thrift::transport::TTransport;
+using apache::thrift::transport::TTransportException;
+
+BOOST_FIXTURE_TEST_SUITE ( TSocketInterruptTest, TestPortFixture )
+
+void readerWorker(boost::shared_ptr<TTransport> tt, uint32_t expectedResult)
+{
+ uint8_t buf[4];
+ BOOST_CHECK_EQUAL(expectedResult, tt->read(buf, 4));
+}
+
+void readerWorkerMustThrow(boost::shared_ptr<TTransport> tt)
+{
+ try
+ {
+ uint8_t buf[4];
+ tt->read(buf, 4);
+ BOOST_ERROR("should not have gotten here");
+ }
+ catch (const TTransportException& tx)
+ {
+ BOOST_CHECK_EQUAL(TTransportException::INTERRUPTED, tx.getType());
+ }
+}
+
+BOOST_AUTO_TEST_CASE( test_interruptable_child_read )
+{
+ TestTServerSocket sock1("localhost", m_serverPort);
+ sock1.listen();
+ TSocket clientSock("localhost", m_serverPort);
+ clientSock.open();
+ boost::shared_ptr<TTransport> accepted = sock1.acceptImpl();
--- End diff --
This should really be 'accept()' and not 'acceptImpl()'.
---
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-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/424#discussion_r28269576
--- Diff: lib/cpp/src/thrift/transport/TSocket.cpp ---
@@ -493,29 +585,9 @@ uint32_t TSocket::read(uint8_t* buf, uint32_t len) {
goto try_again;
}
-#if defined __FreeBSD__ || defined __MACH__
if (errno_copy == THRIFT_ECONNRESET) {
- /* shigin: freebsd doesn't follow POSIX semantic of recv and fails with
- * THRIFT_ECONNRESET if peer performed shutdown
- * edhall: eliminated close() since we do that in the destructor.
- */
return 0;
}
-#endif
-
-#ifdef _WIN32
--- End diff --
I spent a few hours trying to get a MinGW environment set up, but I wasn't able to so I am switching to Cygwin instead. Is there a MinGW 64-bit environment out there that has everything you need for Thrift? MinGW-w64 seemed like it had promise but it was missing the autotool chain.
---
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-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 closed the pull request at:
https://github.com/apache/thrift/pull/424
---
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-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/424#discussion_r28120438
--- Diff: lib/cpp/test/TServerSocketTest.cpp ---
@@ -46,7 +40,8 @@ BOOST_AUTO_TEST_CASE( test_bind_to_address )
accepted->close();
sock1.close();
- TServerSocket sock2("this.is.truly.an.unrecognizable.address.", m_serverPort);
+ std::cout << "An error message from getaddrinfo on the console is expected:" << std::endl;
+ TServerSocket sock2("257.258.259.260", m_serverPort);
--- End diff --
Initially I used a bogus DNS name however it took 15 seconds to fail. This was much faster. :>
---
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-2441 prevent client connections from d...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/424#discussion_r28117469
--- Diff: lib/cpp/src/thrift/server/TThreadPoolServer.h ---
@@ -105,19 +105,16 @@ class TThreadPoolServer : public TServer {
timeout_(0),
taskExpiration_(0) {}
- virtual ~TThreadPoolServer();
+ virtual ~TThreadPoolServer() {}
--- End diff --
This is nit-picky... but I would prefer you revert this particular change. Please leave the virtual dtor out-of-lined. An inlined virtual destructor can cause significant object size bloat. This doesn't end up leaking into final binaries, but it can make build times and file copy times longer.
---
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-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/424#discussion_r28203871
--- 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 removed this logic in THRIFT-3083.
---
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-2441 prevent client connections from d...
Posted by ben-craig <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/424#discussion_r28120466
--- Diff: lib/cpp/test/TSocketInterruptTest.cpp ---
@@ -0,0 +1,147 @@
+/*
+ * 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.
+ */
+
+#define BOOST_TEST_MODULE TSocketInterruptTest
+#include <boost/test/auto_unit_test.hpp>
+
+#include <boost/bind.hpp>
+#include <boost/chrono/duration.hpp>
+#include <boost/date_time/posix_time/posix_time_duration.hpp>
+#include <boost/thread/thread.hpp>
+#include <thrift/transport/TSocket.h>
+#include <thrift/transport/TServerSocket.h>
+#include "TestPortFixture.h"
+#include "TestTServerSocket.h"
+
+using apache::thrift::transport::TServerSocket;
+using apache::thrift::transport::TSocket;
+using apache::thrift::transport::TTransport;
+using apache::thrift::transport::TTransportException;
+
+BOOST_FIXTURE_TEST_SUITE ( TSocketInterruptTest, TestPortFixture )
+
+void readerWorker(boost::shared_ptr<TTransport> tt, uint32_t expectedResult)
+{
+ uint8_t buf[4];
+ BOOST_CHECK_EQUAL(expectedResult, tt->read(buf, 4));
+}
+
+void readerWorkerMustThrow(boost::shared_ptr<TTransport> tt)
+{
+ try
+ {
+ uint8_t buf[4];
+ tt->read(buf, 4);
+ BOOST_ERROR("should not have gotten here");
+ }
+ catch (const TTransportException& tx)
+ {
+ BOOST_CHECK_EQUAL(TTransportException::INTERRUPTED, tx.getType());
+ }
+}
+
+BOOST_AUTO_TEST_CASE( test_interruptable_child_read )
+{
+ TestTServerSocket sock1("localhost", m_serverPort);
+ sock1.listen();
+ TSocket clientSock("localhost", m_serverPort);
+ clientSock.open();
+ boost::shared_ptr<TTransport> accepted = sock1.acceptImpl();
--- End diff --
I am specifically trying to test the implementation here, and not TServerTransport::accept.
---
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-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
GitHub user jeking3 reopened a pull request:
https://github.com/apache/thrift/pull/424
THRIFT-2441 prevent client connections from delaying server stop
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jeking3/thrift bugfix/THRIFT-2441-disconnect-clients-on-server-stop
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/424.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 #424
----
commit 4113f4f50bc4a6a1488b56db14a15e96527c44a7
Author: Jim King <ji...@simplivity.com>
Date: 2015-04-04T19:26:54Z
[THRIFT-2441] prevent client connections from delaying server stop
commit 730877b51d6ef96f84cad34e055f01fd9503aa96
Author: Jim King <ji...@simplivity.com>
Date: 2015-04-04T23:09:16Z
[THRIFT-2441] fix stop handling of simple, threaded server
commit ad68c576a75257dcca26b2c1b88fab27dcd967f0
Author: Jim King <ji...@simplivity.com>
Date: 2015-04-10T04:10:37Z
Merge branch 'master' of github.com:jeking3/thrift into bugfix/THRIFT-2441-disconnect-clients-on-server-stop
commit 251d4d4a2edb3ee95f7b200c70333d7fdc8171c8
Author: Jim King <ji...@simplivity.com>
Date: 2015-04-10T04:44:10Z
THRIFT-2441 addressed code review concerns
commit 0ca766bfd4c8be98481c7295f1bec5e040959ffc
Author: Jim King <ji...@simplivity.com>
Date: 2015-04-12T10:42:21Z
THRIFT-2441 added TServerIntegrationTest to prove combined behavior is correct; fixed TSocket handling of ECONNRESET
----
---
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-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
GitHub user jeking3 reopened a pull request:
https://github.com/apache/thrift/pull/424
THRIFT-2441 prevent client connections from delaying server stop
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jeking3/thrift bugfix/THRIFT-2441-disconnect-clients-on-server-stop
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/424.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 #424
----
commit 4113f4f50bc4a6a1488b56db14a15e96527c44a7
Author: Jim King <ji...@simplivity.com>
Date: 2015-04-04T19:26:54Z
[THRIFT-2441] prevent client connections from delaying server stop
commit 730877b51d6ef96f84cad34e055f01fd9503aa96
Author: Jim King <ji...@simplivity.com>
Date: 2015-04-04T23:09:16Z
[THRIFT-2441] fix stop handling of simple, threaded server
commit ad68c576a75257dcca26b2c1b88fab27dcd967f0
Author: Jim King <ji...@simplivity.com>
Date: 2015-04-10T04:10:37Z
Merge branch 'master' of github.com:jeking3/thrift into bugfix/THRIFT-2441-disconnect-clients-on-server-stop
commit 251d4d4a2edb3ee95f7b200c70333d7fdc8171c8
Author: Jim King <ji...@simplivity.com>
Date: 2015-04-10T04:44:10Z
THRIFT-2441 addressed code review concerns
commit 0ca766bfd4c8be98481c7295f1bec5e040959ffc
Author: Jim King <ji...@simplivity.com>
Date: 2015-04-12T10:42:21Z
THRIFT-2441 added TServerIntegrationTest to prove combined behavior is correct; fixed TSocket handling of ECONNRESET
commit 49153dd33b63d054c72d6e28e5048b2ee80e5cf6
Author: Jim King <ji...@simplivity.com>
Date: 2015-04-12T11:12:46Z
THRIFT-2441 fix cmake error with new integration test using EmptyService
commit e9997d921b7d864bee19ba0b9cf5bb27eb817594
Author: Jim King <ji...@simplivity.com>
Date: 2015-04-12T11:45:49Z
THRIFT-2441 fix cmake build and enhance TServerIntegrationTest to test more code paths
----
---
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-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the pull request:
https://github.com/apache/thrift/pull/424#issuecomment-92031586
Added an integration test to prove behavior is correct; re-opening for CI build.
---
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-2441 prevent client connections from d...
Posted by bufferoverflow <gi...@git.apache.org>.
Github user bufferoverflow commented on a diff in the pull request:
https://github.com/apache/thrift/pull/424#discussion_r28270114
--- Diff: lib/cpp/src/thrift/transport/TSocket.cpp ---
@@ -493,29 +585,9 @@ uint32_t TSocket::read(uint8_t* buf, uint32_t len) {
goto try_again;
}
-#if defined __FreeBSD__ || defined __MACH__
if (errno_copy == THRIFT_ECONNRESET) {
- /* shigin: freebsd doesn't follow POSIX semantic of recv and fails with
- * THRIFT_ECONNRESET if peer performed shutdown
- * edhall: eliminated close() since we do that in the destructor.
- */
return 0;
}
-#endif
-
-#ifdef _WIN32
--- End diff --
Thanks Jim, I'm fine with your explanations, CMake build on appveyor currently fails and I try to get it up and runnning again. I think Windows platform users shall use Visual Studio instead of cygwin and MinGW or as a better choice clang;-)
---
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-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/424#discussion_r28136618
--- 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'm curious, is there anything actually throwing a bare string? I don't see anything, and in general concerned about the inability to debug errors caused by catching too much stuff.
---
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-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/424#discussion_r28120420
--- Diff: lib/cpp/src/thrift/server/TThreadPoolServer.h ---
@@ -105,19 +105,16 @@ class TThreadPoolServer : public TServer {
timeout_(0),
taskExpiration_(0) {}
- virtual ~TThreadPoolServer();
+ virtual ~TThreadPoolServer() {}
--- End diff --
I did this so that it would be the same as other classes in the project, as TSimpleServer and TThreadedServer do this. If I change this I would change all three servers to be the same.
Personally, I don't like having implementations in headers at all even if they are trivial, because when you need to change the implementation (and not the signature of the call) you end up rebuilding too much.
---
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-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the pull request:
https://github.com/apache/thrift/pull/424#issuecomment-92034746
Wow, this really isn't my morning...
---
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-2441 prevent client connections from d...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on the pull request:
https://github.com/apache/thrift/pull/424#issuecomment-91397955
All of the concerns I have with this request are minor. Once these are fixed, I give it a +1. Note that the out-of-line destructor comment applies to the other destructors that got inlined.
I will not personally be able to do the commit for another week or two, but if someone else is able to (after these fixes), then I will be much appreciative.
---
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-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 closed the pull request at:
https://github.com/apache/thrift/pull/424
---
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-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/424#discussion_r28205714
--- Diff: lib/cpp/src/thrift/transport/TSocket.cpp ---
@@ -493,29 +585,9 @@ uint32_t TSocket::read(uint8_t* buf, uint32_t len) {
goto try_again;
}
-#if defined __FreeBSD__ || defined __MACH__
if (errno_copy == THRIFT_ECONNRESET) {
- /* shigin: freebsd doesn't follow POSIX semantic of recv and fails with
- * THRIFT_ECONNRESET if peer performed shutdown
- * edhall: eliminated close() since we do that in the destructor.
- */
return 0;
}
-#endif
-
-#ifdef _WIN32
--- End diff --
Hi Roger, thanks for inspecting this closely. I determined that if one called read() on a closed socket, it was not returning 0 like it should. It turns out that the block seen above which was limiting the logic for handling ECONNRESET to __FreeBSD__ and __MACH__ should be ubiquitous. If a read() errors with ECONNRESET the proper response is to return 0 indicating EOF.
The WIN32 block is the same, given THRIFT_ECONNRESET == WSAECONNRESET when _WIN32 is enabled. As such, now all platforms respond to connection reset during read with 0, meaning EOF.
Does the CI build test framework test Windows and run the CMake tests? If so, then the TServerIntegrationTest will test your concern. I did not run this code on Windows to make sure it builds or passes tests, but I can do that on Monday if needed.
---
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-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the pull request:
https://github.com/apache/thrift/pull/424#issuecomment-95952386
Committed - closing pull request.
---
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-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 closed the pull request at:
https://github.com/apache/thrift/pull/424
---
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-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 closed the pull request at:
https://github.com/apache/thrift/pull/424
---
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-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the pull request:
https://github.com/apache/thrift/pull/424#issuecomment-93028344
Getting a windows build going has been really difficult, which is bizarre because it used to be easy. I've tried MinGW, and cygwin, and native with cmake. Each time I have run into some roadblock that keeps me from making any progress. I want to run the cmake unit tests (make check) on Windows to assist in verifying this change, but it hasn't been easy to do, yet...
---
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-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the pull request:
https://github.com/apache/thrift/pull/424#issuecomment-95177941
To make the (hopefully) impending merge easier, I merged master into this branch and there was one conflict to resolve so I took care of it.
---
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-2441 prevent client connections from d...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/424#discussion_r28117983
--- Diff: lib/cpp/test/TestTServerSocket.h ---
@@ -0,0 +1,31 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#include <thrift/transport/TServerSocket.h>
+
+class TestTServerSocket : public apache::thrift::transport::TServerSocket
--- End diff --
I am confused. Why is this class necessary? It doesn't seem like it is adding anything of use.
I recommend either commenting why you need / want it in the code, or deleting it.
---
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-2441 prevent client connections from d...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/424#discussion_r28117754
--- Diff: lib/cpp/test/TServerSocketTest.cpp ---
@@ -46,7 +40,8 @@ BOOST_AUTO_TEST_CASE( test_bind_to_address )
accepted->close();
sock1.close();
- TServerSocket sock2("this.is.truly.an.unrecognizable.address.", m_serverPort);
+ std::cout << "An error message from getaddrinfo on the console is expected:" << std::endl;
+ TServerSocket sock2("257.258.259.260", m_serverPort);
--- End diff --
Are you sure this IP address isn't real? I think I saw someone use it on a TV show.
/kidding
---
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-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the pull request:
https://github.com/apache/thrift/pull/424#issuecomment-91989278
As part of work on THRIFT-3084 I added a TServer integration test, and found that the shutdown behavior and the rules for when you can call setInterruptableChildren() had to be tightened up for a couple cases. I am going to close this pull request until this is resolved so that it doesn't get merged in until the test passes.
---
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-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the pull request:
https://github.com/apache/thrift/pull/424#issuecomment-92032827
Closing and reopening because three builds failed with "build.ninja not found". Seems unrelated to changes made.
---
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-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 closed the pull request at:
https://github.com/apache/thrift/pull/424
---
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-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the pull request:
https://github.com/apache/thrift/pull/424#issuecomment-95891841
I rebased with master after THRIFT-3000 was reverted to ensure that in Travis CI, job #1 passes.
---
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-2441 prevent client connections from d...
Posted by jeking3 <gi...@git.apache.org>.
GitHub user jeking3 reopened a pull request:
https://github.com/apache/thrift/pull/424
THRIFT-2441 prevent client connections from delaying server stop
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jeking3/thrift bugfix/THRIFT-2441-disconnect-clients-on-server-stop
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/424.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 #424
----
commit 4113f4f50bc4a6a1488b56db14a15e96527c44a7
Author: Jim King <ji...@simplivity.com>
Date: 2015-04-04T19:26:54Z
[THRIFT-2441] prevent client connections from delaying server stop
commit 730877b51d6ef96f84cad34e055f01fd9503aa96
Author: Jim King <ji...@simplivity.com>
Date: 2015-04-04T23:09:16Z
[THRIFT-2441] fix stop handling of simple, threaded server
commit ad68c576a75257dcca26b2c1b88fab27dcd967f0
Author: Jim King <ji...@simplivity.com>
Date: 2015-04-10T04:10:37Z
Merge branch 'master' of github.com:jeking3/thrift into bugfix/THRIFT-2441-disconnect-clients-on-server-stop
commit 251d4d4a2edb3ee95f7b200c70333d7fdc8171c8
Author: Jim King <ji...@simplivity.com>
Date: 2015-04-10T04:44:10Z
THRIFT-2441 addressed code review concerns
commit 0ca766bfd4c8be98481c7295f1bec5e040959ffc
Author: Jim King <ji...@simplivity.com>
Date: 2015-04-12T10:42:21Z
THRIFT-2441 added TServerIntegrationTest to prove combined behavior is correct; fixed TSocket handling of ECONNRESET
commit 49153dd33b63d054c72d6e28e5048b2ee80e5cf6
Author: Jim King <ji...@simplivity.com>
Date: 2015-04-12T11:12:46Z
THRIFT-2441 fix cmake error with new integration test using EmptyService
----
---
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.
---