Skip to content

Put ImR_Activator_ORB_Runner into its own file, use make_unique, fix C++23 warnings #2422

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

Merged
jwillemsen merged 6 commits into from
Jul 9, 2025
Merged

Put ImR_Activator_ORB_Runner into its own file, use make_unique, fix C++23 warnings #2422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
aed671c
Put ImR_Activator_ORB_Runner into its own file, use make_unique
jwillemsen Jul 9, 2025
3375743
Use default/override
jwillemsen Jul 9, 2025
1882aa4
Removed volatile, causes warnings with C++23
jwillemsen Jul 9, 2025
2687d6b
Removed volatile, causes warnings with C++23
jwillemsen Jul 9, 2025
cff6fe2
Add missing end newline
jwillemsen Jul 9, 2025
a89e148
Update Activator_ORB_Runner.cpp
jwillemsen Jul 9, 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
9 changes: 3 additions & 6 deletions ACE/tests/Mem_Map_Test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,22 +21,19 @@
#include "ace/OS_NS_fcntl.h"
#include "ace/OS_Memory.h"


#if !defined (ACE_LACKS_MMAP)

static const char ACE_ALPHABET[] = "abcdefghijklmnopqrstuvwxyz";
static const int LINE_LENGTH = 10;
static const int NUM_LINES = 15;
static constexpr int LINE_LENGTH = 10;
static constexpr int NUM_LINES = 15;

static void
reverse_file (ACE_HANDLE file_handle,
char *array,
size_t size)
{
int count = 0;
// LynxOS 3.0.0/PowerPC needs the volatile qualifier, with -O2
// optimization enabled and without ACE_HAS_INLINE.
volatile size_t i = size;
size_t i = size;
--i;

if (array[i] == '\0')
Expand Down
28 changes: 6 additions & 22 deletions TAO/orbsvcs/ImplRepo_Service/Activator_Loader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,9 @@
#include "orbsvcs/Log_Macros.h"
#include "Activator_Loader.h"
#include "Activator_ORB_Runner.h"
#include "ace/Dynamic_Service.h"
#include "ace/Task.h"

class ImR_Activator_ORB_Runner : public ACE_Task_Base
{
ImR_Activator_Loader& service_;
public:
ImR_Activator_ORB_Runner (ImR_Activator_Loader& service)
: service_ (service)
{
}
virtual int svc ()
{
// Block until service_.fini() calls orb->destroy()
this->service_.run ();
return 0;
}
};

ImR_Activator_Loader::ImR_Activator_Loader ()
ImR_Activator_Loader::~ImR_Activator_Loader ()
{
}

Expand All @@ -45,7 +29,7 @@ ImR_Activator_Loader::init (int argc, ACE_TCHAR *argv[])

// Create a thread in which to run the service
ACE_ASSERT (this->runner_.get () == 0);
this->runner_.reset (new ImR_Activator_ORB_Runner (*this));
this->runner_= std::make_unique<ImR_Activator_ORB_Runner> (*this);
this->runner_->activate ();
}
catch (const CORBA::Exception&)
Expand All @@ -64,7 +48,7 @@ ImR_Activator_Loader::fini ()
int ret = this->service_.fini ();

this->runner_->wait ();
this->runner_.reset (0);
this->runner_.reset (nullptr);
return ret;
}
catch (const CORBA::Exception&)
Expand All @@ -75,8 +59,8 @@ ImR_Activator_Loader::fini ()

CORBA::Object_ptr
ImR_Activator_Loader::create_object (CORBA::ORB_ptr,
int,
ACE_TCHAR **)
int,
ACE_TCHAR **)
{
throw CORBA::NO_IMPLEMENT ();
}
Expand Down
13 changes: 7 additions & 6 deletions TAO/orbsvcs/ImplRepo_Service/Activator_Loader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,16 @@ class ImR_Activator_ORB_Runner;
class Activator_Export ImR_Activator_Loader : public TAO_Object_Loader
{
public:
ImR_Activator_Loader();
ImR_Activator_Loader () = default;
~ImR_Activator_Loader ();

virtual int init (int argc, ACE_TCHAR *argv[]);
int init (int argc, ACE_TCHAR *argv[]) override;

virtual int fini ();
int fini () override;

virtual CORBA::Object_ptr create_object (CORBA::ORB_ptr orb,
int argc,
ACE_TCHAR *argv[]);
CORBA::Object_ptr create_object (CORBA::ORB_ptr orb,
int argc,
ACE_TCHAR *argv[]) override;

// Unlike other service objects, we have our own orb.
int run();
Expand Down
15 changes: 15 additions & 0 deletions TAO/orbsvcs/ImplRepo_Service/Activator_ORB_Runner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#include "Activator_ORB_Runner.h"
#include "Activator_Loader.h"

ImR_Activator_ORB_Runner::ImR_Activator_ORB_Runner (ImR_Activator_Loader& service)
: service_ (service)
{
}

int
ImR_Activator_ORB_Runner::svc ()
{
// Block until service_.fini() calls orb->destroy()
this->service_.run ();
return 0;
}
24 changes: 24 additions & 0 deletions TAO/orbsvcs/ImplRepo_Service/Activator_ORB_Runner.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// -*- C++ -*-
#ifndef TAO_IMR_ACTIVATOR_ORB_RUNNER_H
#define TAO_IMR_ACTIVATOR_ORB_RUNNER_H

#include "ace/Task.h"

#if !defined (ACE_LACKS_PRAGMA_ONCE)
# pragma once
#endif /* ACE_LACKS_PRAGMA_ONCE */

class ImR_Activator_Loader;

class ImR_Activator_ORB_Runner : public ACE_Task_Base
{
public:
ImR_Activator_ORB_Runner (ImR_Activator_Loader& service);
~ImR_Activator_ORB_Runner () = default;

int svc () override;
private:
ImR_Activator_Loader& service_;
};

#endif
1 change: 1 addition & 0 deletions TAO/orbsvcs/ImplRepo_Service/ImplRepo_Service.mpc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ project(ImR_Activator) : orbsvcslib, orbsvcs_output, install_lib, acexml, avoids
ImR_Activator_i.cpp
Activator_Options.cpp
Activator_Loader.cpp
Activator_ORB_Runner.cpp
}
header_files {
activator_export.h
Expand Down
10 changes: 1 addition & 9 deletions TAO/orbsvcs/examples/ImR/Combined_Service/dynserver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,6 @@
using namespace CORBA;
using namespace PortableServer;

DynServer::DynServer()
: n_(0)
{
}

DynServer::~DynServer() {
}

Long DynServer::get()
{
ACE_DEBUG((LM_DEBUG, "dynserver: get() %d\n", ++n_));
Expand Down Expand Up @@ -59,7 +51,7 @@ class DynServer_ORB_Runner : public ACE_Task_Base
}
};

DynServer_Loader::DynServer_Loader()
DynServer_Loader::~DynServer_Loader ()
{
}

Expand Down
25 changes: 13 additions & 12 deletions TAO/orbsvcs/examples/ImR/Combined_Service/dynserver.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,14 @@
#include <memory>

// Trivial test corba object
class DynServer_Export DynServer
: public POA_test
class DynServer_Export DynServer : public POA_test
{
int n_;
public:
DynServer();
virtual ~DynServer();
virtual CORBA::Long get();
DynServer () = default;
~DynServer() override = default;
CORBA::Long get() override;
private:
int n_ {};
};

class DynServer_ORB_Runner;
Expand All @@ -26,19 +26,20 @@ class DynServer_ORB_Runner;
class DynServer_Export DynServer_Loader : public TAO_Object_Loader
{
public:
DynServer_Loader();
DynServer_Loader() = default;
~DynServer_Loader ();

// spawns a thread to run an internal orb which has activated
// a single DynServer servant.
virtual int init (int argc, ACE_TCHAR *argv[]);
int init (int argc, ACE_TCHAR *argv[]) override;

// Allows the service configurator to shutdown the orb
virtual int fini ();
int fini () override;

// Not supported
virtual CORBA::Object_ptr create_object (CORBA::ORB_ptr orb,
int argc,
ACE_TCHAR *argv[]);
CORBA::Object_ptr create_object (CORBA::ORB_ptr orb,
int argc,
ACE_TCHAR *argv[]) override;

private:
CORBA::ORB_var orb_;
Expand Down
6 changes: 3 additions & 3 deletions TAO/tests/MT_NoUpcall_Client_Leader/chatter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@ class Chatter : public Worker
{
public:
Chatter (CORBA::ORB_ptr orb, const ACE_TCHAR *ior, ACE_Condition<ACE_Mutex>& cond);
virtual int svc ();
int svc () override;
int farewell ();
long nrequests ();
long nreplies ();
public:
volatile long nrequests_;
volatile long nreplies_;
long nrequests_;
long nreplies_;
Comment on lines +18 to +19

@coderabbitai coderabbitai Bot Jul 9, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Verify thread safety after removing volatile qualifier.

The removal of volatile from these member variables could introduce race conditions if they're accessed from multiple threads without proper synchronization. While volatile is often misused for thread synchronization, its removal needs careful verification.

Since this is multi-threaded code (MT prefix) and the variables are public, ensure that:

  1. All access to these variables is properly synchronized using the request_reply_count_mutex_
  2. The nrequests() and nreplies() methods provide thread-safe access
  3. No direct access to these variables bypasses the synchronization

🏁 Script executed:

#!/bin/bash
# Description: Verify thread-safe access patterns for nrequests_ and nreplies_ variables
# Expected: All access should be synchronized through methods or protected by mutex

# Check implementation of accessor methods
rg -A 10 "long.*Chatter::nrequests\(\)" --type cpp

# Check for any direct access to these variables
rg -A 3 -B 3 "nrequests_|nreplies_" --type cpp --type h

# Look for mutex usage patterns
rg -A 5 -B 5 "request_reply_count_mutex_" --type cpp

Length of output: 5908

Ensure all reads and writes of nrequests_/nreplies_ are synchronized

The current implementation protects increments with request_reply_count_mutex_, but:

  • The accessors Chatter::nrequests() and Chatter::nreplies() return the raw fields without locking.
  • In client.cpp, there’s a direct read of the public members (worker2.nrequests_, worker2.nreplies_) bypassing any mutex.

These gaps can lead to data races. Please address the following:

• Guard the accessors with the same mutex:

--- a/TAO/tests/MT_NoUpcall_Client_Leader/chatter.cpp
+++ b/TAO/tests/MT_NoUpcall_Client_Leader/chatter.cpp
@@ -1,4 +1,9 @@
-long Chatter::nrequests () { return this->nrequests_; }
+long Chatter::nrequests ()
+{
+  ACE_GUARD_RETURN (TAO_SYNCH_MUTEX, guard, request_reply_count_mutex_, 0);
+  return this->nrequests_;
+}

-long Chatter::nreplies ()  { return this->nreplies_; }
+long Chatter::nreplies ()
+{
+  ACE_GUARD_RETURN (TAO_SYNCH_MUTEX, guard, request_reply_count_mutex_, 0);
+  return this->nreplies_;
+}

• Replace any direct member access in client.cpp with the thread-safe accessors:

--- a/TAO/tests/MT_NoUpcall_Client_Leader/client.cpp
+++ b/TAO/tests/MT_NoUpcall_Client_Leader/client.cpp
@@ -XX,7 +XX,7 @@
- result = (worker2.nrequests_ == worker2.nreplies_) ? 0 : -1;
+ result = (worker2.nrequests() == worker2.nreplies()) ? 0 : -1;

• (Optional) Make nrequests_ and nreplies_ private in chatter.h to prevent accidental direct access.

Implementing these changes will eliminate potential race conditions when reading or writing the counters.

🤖 Prompt for AI Agents 
In TAO/tests/MT_NoUpcall_Client_Leader/chatter.h around lines 18-19, the fields
nrequests_ and nreplies_ are accessed without synchronization, causing potential
data races. To fix this, make nrequests_ and nreplies_ private, protect their
accessors by locking request_reply_count_mutex_ before returning the values, and
update client.cpp to replace any direct access to these members with calls to
the thread-safe accessor methods.

private:
const ACE_TCHAR* ior_;
ACE_Condition<ACE_Mutex>& cond_;
Expand Down