Skip to content

THRIFT-5898: Replace global variable with a singleton getter #3226

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

THRIFT-5898: Replace global variable with a singleton getter #3226

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7171537
THRIFT-5898: Replace global variable with a singleton getter
CJCombrink Nov 3, 2025
ec5d2b8
Moving the body of setManualOpenSSLInitialization() to the cpp file
CJCombrink Nov 3, 2025
585f471
Put back Windows shared lib building
CJCombrink Nov 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion appveyor.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ environment:
APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2017
PLATFORM: x64
CONFIGURATION: Release
BUILD_SHARED_LIBS: OFF
BUILD_SHARED_LIBS: ON
Copy link
Member

@emmenlau emmenlau Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be in a separate PR, to add shared library build? This PR here is 99% only about replacing GlobalOutput, which is useful to me independent of shared libraries.

Copy link
Contributor Author

@CJCombrink CJCombrink Nov 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is actually related: In #3229 I disabled building the shared libraries due to this PR still outstanding with the mention there that I would put it back, see b1e7cfb

Also building a shared library was broken and this is (my) the proposed solution to said problem.

Copy link
Contributor Author

@CJCombrink CJCombrink Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To follow up on mail conversation:
The CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS does not export global symbols according to the cmake docs

However, there are some cases when compiling code in a consumer may require explicit dllimport markup:

  • Global data symbols must be explicitly marked with __declspec(dllimport) in order to link to data in the .dll.

BOOST_VERSION: 1.67.0
LIBEVENT_VERSION: 2.1.8
PYTHON_VERSION: 3.6
Expand Down
4 changes: 2 additions & 2 deletions contrib/transport-sample/ThriftCommon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace thriftcommon
{
//----------------------------------------------------------------------------
//Launch child process and pass R/W anonymous pipe handles on cmd line.
//This is a simple example and does not include elevation or other
//This is a simple example and does not include elevation or other
//advanced features.
//
bool LaunchAnonPipeChild(std::string app, boost::shared_ptr<TServerTransport> transport)
Expand All @@ -25,7 +25,7 @@ namespace thriftcommon
//spawn the child process
if (!CreateProcessA(nullptr, handles, nullptr,nullptr,TRUE,0,nullptr,nullptr,&si,&pi))
{
GlobalOutput.perror("TPipeServer CreateProcess failed, GLE=", GetLastError());
TOutput::instance().perror("TPipeServer CreateProcess failed, GLE=", GetLastError());
return false;
}

Expand Down
6 changes: 3 additions & 3 deletions lib/cpp/src/thrift/TDispatchProcessor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ class TDispatchProcessorT : public TProcessor {
// (The old generated processor code used to try to skip a T_STRUCT and
// continue. However, that seems unsafe.)
if (mtype != protocol::T_CALL && mtype != protocol::T_ONEWAY) {
GlobalOutput.printf("received invalid message type %d from client", mtype);
TOutput::instance().printf("received invalid message type %d from client", mtype);
return false;
}

Expand All @@ -76,7 +76,7 @@ class TDispatchProcessorT : public TProcessor {
in->readMessageBegin(fname, mtype, seqid);

if (mtype != protocol::T_CALL && mtype != protocol::T_ONEWAY) {
GlobalOutput.printf("received invalid message type %d from client", mtype);
TOutput::instance().printf("received invalid message type %d from client", mtype);
return false;
}

Expand Down Expand Up @@ -114,7 +114,7 @@ class TDispatchProcessor : public TProcessor {
in->readMessageBegin(fname, mtype, seqid);

if (mtype != protocol::T_CALL && mtype != protocol::T_ONEWAY) {
GlobalOutput.printf("received invalid message type %d from client", mtype);
TOutput::instance().printf("received invalid message type %d from client", mtype);
return false;
}

Expand Down
8 changes: 6 additions & 2 deletions lib/cpp/src/thrift/TOutput.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,6 @@
namespace apache {
namespace thrift {

/*THRIFT_EXPORT*/ TOutput GlobalOutput; // if you need this exported, build your own wrapper lib around and export it yourself

TOutput::TOutput() : f_(&errorTimeWrapper) {}

void TOutput::printf(const char* message, ...) {
Expand Down Expand Up @@ -148,5 +146,11 @@ std::string TOutput::strerror_s(int errno_copy) {
return std::string(b_error);
#endif // __ZEPHYR__
}

TOutput& TOutput::instance() {
static TOutput instance;
return instance;
}

}
} // apache::thrift
8 changes: 4 additions & 4 deletions lib/cpp/src/thrift/TOutput.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@
#ifndef _THRIFT_OUTPUT_H_
#define _THRIFT_OUTPUT_H_ 1

//#include <thrift/thrift_export.h>

namespace apache {
namespace thrift {

Expand Down Expand Up @@ -49,11 +47,13 @@ class TOutput {
/** Just like strerror_r but returns a C++ string object. */
static std::string strerror_s(int errno_copy);

/** Get a singleton instance of the global TOutput object used by
* the library internally. */
static TOutput& instance();

private:
void (*f_)(const char*);
};

/*THRIFT_EXPORT*/ extern TOutput GlobalOutput; // if you need this exported, build your own wrapper lib around and export it yourself
}
} // namespace apache::thrift

Expand Down
6 changes: 3 additions & 3 deletions lib/cpp/src/thrift/async/TAsyncDispatchProcessor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ class TAsyncDispatchProcessorT : public TAsyncProcessor {
// (The old generated processor code used to try to skip a T_STRUCT and
// continue. However, that seems unsafe.)
if (mtype != protocol::T_CALL && mtype != protocol::T_ONEWAY) {
GlobalOutput.printf("received invalid message type %d from client", mtype);
TOutput::instance().printf("received invalid message type %d from client", mtype);
_return(false);
return;
}
Expand All @@ -79,7 +79,7 @@ class TAsyncDispatchProcessorT : public TAsyncProcessor {
in->readMessageBegin(fname, mtype, seqid);

if (mtype != protocol::T_CALL && mtype != protocol::T_ONEWAY) {
GlobalOutput.printf("received invalid message type %d from client", mtype);
TOutput::instance().printf("received invalid message type %d from client", mtype);
_return(false);
return;
}
Expand Down Expand Up @@ -123,7 +123,7 @@ class TAsyncDispatchProcessor : public TAsyncProcessor {
// (The old generated processor code used to try to skip a T_STRUCT and
// continue. However, that seems unsafe.)
if (mtype != protocol::T_CALL && mtype != protocol::T_ONEWAY) {
GlobalOutput.printf("received invalid message type %d from client", mtype);
TOutput::instance().printf("received invalid message type %d from client", mtype);
_return(false);
return;
}
Expand Down
4 changes: 2 additions & 2 deletions lib/cpp/src/thrift/concurrency/ThreadManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,9 +309,9 @@ class ThreadManager::Worker : public Runnable {
try {
task->run();
} catch (const std::exception& e) {
GlobalOutput.printf("[ERROR] task->run() raised an exception: %s", e.what());
TOutput::instance().printf("[ERROR] task->run() raised an exception: %s", e.what());
} catch (...) {
GlobalOutput.printf("[ERROR] task->run() raised an unknown exception");
TOutput::instance().printf("[ERROR] task->run() raised an unknown exception");
}

// Re-acquire the lock to proceed in the thread manager
Expand Down
10 changes: 5 additions & 5 deletions lib/cpp/src/thrift/server/TConnectedClient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,14 @@ void TConnectedClient::run() {
// All other transport exceptions are logged.
// State of connection is unknown. Done.
string errStr = string("TConnectedClient died: ") + ttx.what();
GlobalOutput(errStr.c_str());
TOutput::instance()(errStr.c_str());
done = true;
break;
}
}
} catch (const TException& tex) {
string errStr = string("TConnectedClient processing exception: ") + tex.what();
GlobalOutput(errStr.c_str());
TOutput::instance()(errStr.c_str());
// Disconnect from client, because we could not process the message.
done = true;
}
Expand All @@ -100,21 +100,21 @@ void TConnectedClient::cleanup() {
inputProtocol_->getTransport()->close();
} catch (const TTransportException& ttx) {
string errStr = string("TConnectedClient input close failed: ") + ttx.what();
GlobalOutput(errStr.c_str());
TOutput::instance()(errStr.c_str());
}

try {
outputProtocol_->getTransport()->close();
} catch (const TTransportException& ttx) {
string errStr = string("TConnectedClient output close failed: ") + ttx.what();
GlobalOutput(errStr.c_str());
TOutput::instance()(errStr.c_str());
}

try {
client_->close();
} catch (const TTransportException& ttx) {
string errStr = string("TConnectedClient client close failed: ") + ttx.what();
GlobalOutput(errStr.c_str());
TOutput::instance()(errStr.c_str());
}
}
}
Expand Down
Loading