[
https://issues.apache.org/jira/browse/THRIFT-4384?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16264278#comment-16264278
]
Michael Eiler edited comment on THRIFT-4384 at 11/23/17 12:49 PM:
------------------------------------------------------------------
Currently I solved the problem for us like shown here:
{code:none}
diff --git a/compiler/cpp/src/thrift/generate/t_cpp_generator.cc
b/compiler/cpp/src/thrift/generate/t_cpp_generator.cc
index cbe8da2..515e0e2 100644
--- a/compiler/cpp/src/thrift/generate/t_cpp_generator.cc
+++ b/compiler/cpp/src/thrift/generate/t_cpp_generator.cc
@@ -1616,6 +1616,8 @@ void t_cpp_generator::generate_service(t_service*
tservice) {
<< "namespace apache { namespace thrift { namespace async {" <<
endl
<< "class TAsyncChannel;" << endl << "}}}" << endl;
}
+ f_header_ << "#include <boost/make_shared.hpp>" << endl;
+ f_header_ << "#include <boost/shared_ptr.hpp>" << endl;
f_header_ << "#include <thrift/TDispatchProcessor.h>" << endl;
if (gen_cob_style_) {
f_header_ << "#include <thrift/async/TAsyncDispatchProcessor.h>" << endl;
@@ -2169,7 +2171,14 @@ void t_cpp_generator::generate_service_client(t_service*
tservice, string style)
indent_up();
if (style != "Cob") {
f_header_ << indent() << service_name_ << style << "Client" <<
short_suffix << "(" << prot_ptr
- << " prot) ";
+ << " prot";
+ if (style == "Concurrent") {
+ f_header_ << ",
boost::shared_ptr<::apache::thrift::async::TConcurrentClientSyncInfo> sync =
boost::make_shared<::apache::thrift::async::TConcurrentClientSyncInfo>()"
+ << ") : sync_(sync) ";
+ }
+ else {
+ f_header_ << ") ";
+ }
if (extends.empty()) {
f_header_ << "{" << endl;
@@ -2182,7 +2191,15 @@ void t_cpp_generator::generate_service_client(t_service*
tservice, string style)
}
f_header_ << indent() << service_name_ << style << "Client" <<
short_suffix << "(" << prot_ptr
- << " iprot, " << prot_ptr << " oprot) ";
+ << " iprot, " << prot_ptr << " oprot";
+ if (style == "Concurrent") {
+ f_header_ << ",
boost::shared_ptr<::apache::thrift::async::TConcurrentClientSyncInfo> sync =
boost::make_shared<::apache::thrift::async::TConcurrentClientSyncInfo>()"
+ << ") : sync_(sync) ";
+ }
+ else {
+ f_header_ << ") ";
+ }
+
if (extends.empty()) {
f_header_ << "{" << endl;
f_header_ << indent() << " setProtocol" << short_suffix <<
"(iprot,oprot);" << endl
@@ -2328,7 +2345,7 @@ void t_cpp_generator::generate_service_client(t_service*
tservice, string style)
if (style == "Concurrent") {
f_header_ <<
- indent() << "::apache::thrift::async::TConcurrentClientSyncInfo
sync_;"<<endl;
+ indent() <<
"boost::shared_ptr<::apache::thrift::async::TConcurrentClientSyncInfo>
sync_;"<<endl;
}
indent_down();
}
@@ -2434,7 +2451,7 @@ void t_cpp_generator::generate_service_client(t_service*
tservice, string style)
string cseqidVal = "0";
if (style == "Concurrent") {
if (!(*f_iter)->is_oneway()) {
- cseqidVal = "this->sync_.generateSeqId()";
+ cseqidVal = "this->sync_->generateSeqId()";
}
}
// Serialize the request
@@ -2442,7 +2459,7 @@ void t_cpp_generator::generate_service_client(t_service*
tservice, string style)
indent() << "int32_t cseqid = " << cseqidVal << ";" << endl;
if(style == "Concurrent") {
out <<
- indent() << "::apache::thrift::async::TConcurrentSendSentry
sentry(&this->sync_);" << endl;
+ indent() << "::apache::thrift::async::TConcurrentSendSentry
sentry(this->sync_.get());" << endl;
}
if (style == "Cob") {
out <<
@@ -2507,7 +2524,7 @@ void t_cpp_generator::generate_service_client(t_service*
tservice, string style)
endl <<
indent() << "// the read mutex gets dropped and reacquired as part
of waitForWork()" << endl <<
indent() << "// The destructor of this sentry wakes up other
clients" << endl <<
- indent() << "::apache::thrift::async::TConcurrentRecvSentry
sentry(&this->sync_, seqid);" << endl;
+ indent() << "::apache::thrift::async::TConcurrentRecvSentry
sentry(this->sync_.get(), seqid);" << endl;
}
if (style == "Cob" && !gen_no_client_completion_) {
out << indent() << "bool completed = false;" << endl << endl <<
indent() << "try {";
@@ -2517,7 +2534,7 @@ void t_cpp_generator::generate_service_client(t_service*
tservice, string style)
if (style == "Concurrent") {
out <<
indent() << "while(true) {" << endl <<
- indent() << " if(!this->sync_.getPending(fname, mtype, rseqid))
{" << endl;
+ indent() << " if(!this->sync_->getPending(fname, mtype, rseqid))
{" << endl;
indent_up();
indent_up();
}
@@ -2661,10 +2678,10 @@ void
t_cpp_generator::generate_service_client(t_service* tservice, string style)
out <<
indent() << " }" << endl <<
indent() << " // seqid != rseqid" << endl <<
- indent() << " this->sync_.updatePending(fname, mtype, rseqid);"
<< endl <<
+ indent() << " this->sync_->updatePending(fname, mtype, rseqid);"
<< endl <<
endl <<
indent() << " // this will temporarily unlock the readMutex, and
let other clients get work done" << endl <<
- indent() << " this->sync_.waitForWork(seqid);" << endl <<
+ indent() << " this->sync_->waitForWork(seqid);" << endl <<
indent() << "} // end while(true)" << endl;
}
if (style == "Cob" && !gen_no_client_completion_) {
{code}
(
https://github.com/MichaelE1000/thrift/commit/44ec66e550d90cf8e0074cd46769b5be0d29253b
)
Do you see any issues with this approach?
Best Regards
was (Author: michaele1000):
Currently I solved the problem for us like shown here:
{code:patch}
diff --git a/compiler/cpp/src/thrift/generate/t_cpp_generator.cc
b/compiler/cpp/src/thrift/generate/t_cpp_generator.cc
index cbe8da2..515e0e2 100644
--- a/compiler/cpp/src/thrift/generate/t_cpp_generator.cc
+++ b/compiler/cpp/src/thrift/generate/t_cpp_generator.cc
@@ -1616,6 +1616,8 @@ void t_cpp_generator::generate_service(t_service*
tservice) {
<< "namespace apache { namespace thrift { namespace async {" <<
endl
<< "class TAsyncChannel;" << endl << "}}}" << endl;
}
+ f_header_ << "#include <boost/make_shared.hpp>" << endl;
+ f_header_ << "#include <boost/shared_ptr.hpp>" << endl;
f_header_ << "#include <thrift/TDispatchProcessor.h>" << endl;
if (gen_cob_style_) {
f_header_ << "#include <thrift/async/TAsyncDispatchProcessor.h>" << endl;
@@ -2169,7 +2171,14 @@ void t_cpp_generator::generate_service_client(t_service*
tservice, string style)
indent_up();
if (style != "Cob") {
f_header_ << indent() << service_name_ << style << "Client" <<
short_suffix << "(" << prot_ptr
- << " prot) ";
+ << " prot";
+ if (style == "Concurrent") {
+ f_header_ << ",
boost::shared_ptr<::apache::thrift::async::TConcurrentClientSyncInfo> sync =
boost::make_shared<::apache::thrift::async::TConcurrentClientSyncInfo>()"
+ << ") : sync_(sync) ";
+ }
+ else {
+ f_header_ << ") ";
+ }
if (extends.empty()) {
f_header_ << "{" << endl;
@@ -2182,7 +2191,15 @@ void t_cpp_generator::generate_service_client(t_service*
tservice, string style)
}
f_header_ << indent() << service_name_ << style << "Client" <<
short_suffix << "(" << prot_ptr
- << " iprot, " << prot_ptr << " oprot) ";
+ << " iprot, " << prot_ptr << " oprot";
+ if (style == "Concurrent") {
+ f_header_ << ",
boost::shared_ptr<::apache::thrift::async::TConcurrentClientSyncInfo> sync =
boost::make_shared<::apache::thrift::async::TConcurrentClientSyncInfo>()"
+ << ") : sync_(sync) ";
+ }
+ else {
+ f_header_ << ") ";
+ }
+
if (extends.empty()) {
f_header_ << "{" << endl;
f_header_ << indent() << " setProtocol" << short_suffix <<
"(iprot,oprot);" << endl
@@ -2328,7 +2345,7 @@ void t_cpp_generator::generate_service_client(t_service*
tservice, string style)
if (style == "Concurrent") {
f_header_ <<
- indent() << "::apache::thrift::async::TConcurrentClientSyncInfo
sync_;"<<endl;
+ indent() <<
"boost::shared_ptr<::apache::thrift::async::TConcurrentClientSyncInfo>
sync_;"<<endl;
}
indent_down();
}
@@ -2434,7 +2451,7 @@ void t_cpp_generator::generate_service_client(t_service*
tservice, string style)
string cseqidVal = "0";
if (style == "Concurrent") {
if (!(*f_iter)->is_oneway()) {
- cseqidVal = "this->sync_.generateSeqId()";
+ cseqidVal = "this->sync_->generateSeqId()";
}
}
// Serialize the request
@@ -2442,7 +2459,7 @@ void t_cpp_generator::generate_service_client(t_service*
tservice, string style)
indent() << "int32_t cseqid = " << cseqidVal << ";" << endl;
if(style == "Concurrent") {
out <<
- indent() << "::apache::thrift::async::TConcurrentSendSentry
sentry(&this->sync_);" << endl;
+ indent() << "::apache::thrift::async::TConcurrentSendSentry
sentry(this->sync_.get());" << endl;
}
if (style == "Cob") {
out <<
@@ -2507,7 +2524,7 @@ void t_cpp_generator::generate_service_client(t_service*
tservice, string style)
endl <<
indent() << "// the read mutex gets dropped and reacquired as part
of waitForWork()" << endl <<
indent() << "// The destructor of this sentry wakes up other
clients" << endl <<
- indent() << "::apache::thrift::async::TConcurrentRecvSentry
sentry(&this->sync_, seqid);" << endl;
+ indent() << "::apache::thrift::async::TConcurrentRecvSentry
sentry(this->sync_.get(), seqid);" << endl;
}
if (style == "Cob" && !gen_no_client_completion_) {
out << indent() << "bool completed = false;" << endl << endl <<
indent() << "try {";
@@ -2517,7 +2534,7 @@ void t_cpp_generator::generate_service_client(t_service*
tservice, string style)
if (style == "Concurrent") {
out <<
indent() << "while(true) {" << endl <<
- indent() << " if(!this->sync_.getPending(fname, mtype, rseqid))
{" << endl;
+ indent() << " if(!this->sync_->getPending(fname, mtype, rseqid))
{" << endl;
indent_up();
indent_up();
}
@@ -2661,10 +2678,10 @@ void
t_cpp_generator::generate_service_client(t_service* tservice, string style)
out <<
indent() << " }" << endl <<
indent() << " // seqid != rseqid" << endl <<
- indent() << " this->sync_.updatePending(fname, mtype, rseqid);"
<< endl <<
+ indent() << " this->sync_->updatePending(fname, mtype, rseqid);"
<< endl <<
endl <<
indent() << " // this will temporarily unlock the readMutex, and
let other clients get work done" << endl <<
- indent() << " this->sync_.waitForWork(seqid);" << endl <<
+ indent() << " this->sync_->waitForWork(seqid);" << endl <<
indent() << "} // end while(true)" << endl;
}
if (style == "Cob" && !gen_no_client_completion_) {
{code}
(
https://github.com/MichaelE1000/thrift/commit/44ec66e550d90cf8e0074cd46769b5be0d29253b
)
Do you see any issues with this approach?
Best Regards
> Using multiple services simultaneously is not thread-safe.
> ----------------------------------------------------------
>
> Key: THRIFT-4384
> URL: https://issues.apache.org/jira/browse/THRIFT-4384
> Project: Thrift
> Issue Type: Bug
> Components: C++ - Compiler, C++ - Library
> Affects Versions: 0.10.0
> Environment: Should affect all platforms but has been noticed first
> on Windows, x86_64.
> Reporter: Michael Eiler
> Priority: Critical
> Fix For: 0.10.0, 0.11.0
>
>
> I'm using the generated *ServiceConcurrentClient classes. They should allow
> me to call multiple functions at the same time.
> The issue as that the ::apache::thrift::async::TConcurrentClientSyncInfo
> class is a member of the generated service. If I have a project with multiple
> services sharing the same connection (protocol) with each other, the services
> will not be mutually excluded from reading on the same socket.
> I did a small test with patching the generated code and injecting the same
> instance of TConcurrentClientSyncInfo into all my services and everything was
> fine.
> Question: Do you need a small project to reproduce this or is it obvious
> enough? Just check out any generated code and you will see that the
> TConcurrentClientSyncInfo is not shared between different services.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
