[ace6tao2] Backport #2523, add -ORBTransportIdleTimeout to purge idle transports

[jwillemsen]

Summary by CodeRabbit

Release Notes

• New Features

  • Added configurable -ORBTransportIdleTimeout option to automatically close and purge idle transport connections after a specified timeout period (in seconds). Default is 0, meaning no idle timeout.

• Tests

  • Added comprehensive regression tests validating idle transport timeout behavior across multiple scenarios.

• Documentation

  • Updated configuration documentation with details for the new transport idle timeout option.

• Chores

  • Minor code refinements and type improvements.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: a1a96148-b629-47ae-a0a7-a37b1b587a73

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (5)

ACE/ace/Event_Handler_Handle_Timeout_Upcall.cpp (1)

32-32: Consider making the variable const for consistency.

The variable is assigned once and never modified. For consistency with similar changes in WFMO_Reactor.cpp (lines 873, 2065), consider using bool const instead of bool.

Additionally, the similar variable at line 83 (int requires_reference_counting in the cancel_type function) was not updated and remains an int.

♻️ Proposed change to add const qualifier

-  bool requires_reference_counting = false;
+  bool const requires_reference_counting = false;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ACE/ace/Event_Handler_Handle_Timeout_Upcall.cpp` at line 32, The local flag
requires_reference_counting is assigned once and should be made const for
consistency: change the declaration "bool requires_reference_counting = false;"
in Event_Handler_Handle_Timeout_Upcall.cpp to use a const-qualified type (e.g.,
bool const requires_reference_counting = false;) and likewise update the similar
declaration in the cancel_type function (currently "int
requires_reference_counting") to "int const requires_reference_counting" so both
variables are immutable and consistent with the WFMO_Reactor changes.

ACE/ace/WFMO_Reactor.cpp (1)

2065-2067: Standardize variable naming and type for reference counting checks across dispatch handlers.

The variable in simple_dispatch_handler (line 2065) correctly uses bool const requires_reference_counting, but complex_dispatch_handler (lines 2122-2124) uses int reference_counting_required with an int type instead. This inconsistency creates unnecessary confusion and reduces code clarity. Consider renaming and retyping the variable in complex_dispatch_handler to match the pattern used in simple_dispatch_handler and other functions in the codebase.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ACE/ace/WFMO_Reactor.cpp` around lines 2065 - 2067, The
complex_dispatch_handler uses an int named reference_counting_required for the
reference-counting check which is inconsistent with simple_dispatch_handler;
change the variable in complex_dispatch_handler to match the pattern: declare a
bool const requires_reference_counting and set it by comparing
event_handler->reference_counting_policy().value() ==
ACE_Event_Handler::Reference_Counting_Policy::ENABLED (same expression used in
simple_dispatch_handler), update any subsequent uses of the old variable name to
the new name, and remove the int declaration to standardize naming and type
across dispatch handlers.

TAO/docs/Options.html (1)

1393-1396: Document 0 as the explicit disable value.

Lines 1393-1396 describe default behavior, but explicitly stating 0 disables idle-timeout purging would align this text with the API contract and remove ambiguity.

📝 Suggested doc tweak

-        <td>Number of seconds after which idle connections are closed and purged from
+        <td>Number of seconds after which idle connections are closed and purged from
         the transport cache. By default idle connections are kept until they are
-        explicitly closed or purged because the transport cache is almost full.</td>
+        explicitly closed or purged because the transport cache is almost full.
+        A value of <code>0</code> disables idle-timeout purging.</td>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@TAO/docs/Options.html` around lines 1393 - 1396, Update the documentation for
the -ORBTransportIdleTimeout option to explicitly state that a value of 0
disables idle-timeout purging; modify the description for
-ORBTransportIdleTimeout so it still explains default behavior but adds a
sentence like “A value of 0 disables idle-timeout purging (connections are never
closed based on idle timeout).” to make the API contract unambiguous.

TAO/tests/Transport_Idle_Timeout/client_multiple.cpp (1)

297-297: Minor: Double semicolon.

-      Test::Echo_var echo2 = Test::Echo::_narrow (obj2.in ());;
+      Test::Echo_var echo2 = Test::Echo::_narrow (obj2.in ());

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@TAO/tests/Transport_Idle_Timeout/client_multiple.cpp` at line 297, The line
initializing echo2 has a stray double semicolon: "Test::Echo_var echo2 =
Test::Echo::_narrow (obj2.in ());;". Remove the extra trailing semicolon so the
statement ends with a single semicolon; locate this in the Test::Echo_var echo2
initialization (the _narrow call) and correct it.

TAO/tests/Transport_Idle_Timeout/client.cpp (1)

107-107: Minor: Unused option -l in single-server client.

The option string includes l: but this client doesn't use the -l option (that's for client_multiple.cpp). While harmless, removing it would clarify the interface.

-  ACE_Get_Opt get_opts (argc, argv, ACE_TEXT ("k:l:t:n:d"));
+  ACE_Get_Opt get_opts (argc, argv, ACE_TEXT ("k:t:n:d"));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@TAO/tests/Transport_Idle_Timeout/client.cpp` at line 107, The ACE_Get_Opt
option string passed to ACE_Get_Opt (ACE_Get_Opt get_opts (argc, argv, ACE_TEXT
("k:l:t:n:d"))); includes an unused 'l:' option; remove 'l:' from the string so
it becomes ACE_TEXT("k:t:n:d") to match the single-server client CLI and avoid
confusion, and update any related parsing/switch cases in the same scope (e.g.,
the ACE_Get_Opt instance and its switch handling) to ensure no case for 'l'
remains.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@ACE/ace/WFMO_Reactor.cpp`:
- Around line 873-875: Change the declaration of requires_reference_counting in
make_changes_in_to_be_added_infos from an int to a bool const so it matches the
other functions; locate the requires_reference_counting variable inside the
make_changes_in_to_be_added_infos function and replace its type with "bool
const" (keeping the existing initializer:
event_handler->reference_counting_policy().value() ==
ACE_Event_Handler::Reference_Counting_Policy::ENABLED) to ensure consistent
typing with make_changes_in_current_infos and make_changes_in_suspension_infos.

In `@TAO/tao/default_resource.cpp`:
- Around line 490-497: The error path in the argv parsing for
"-ORBTransportIdleTimeout" reads argv[curarg] when curarg >= argc and also
allows negative timeouts; update the parsing in tao::Default_Resource (the
branch handling ACE_OS::strcasecmp(..., "-ORBTransportIdleTimeout")) to first
verify curarg < argc before any access and call report_option_value_error with a
safe sentinel (or nullptr/message) when missing, and after parsing via
ACE_OS::atoi validate the resulting value is non-negative before assigning to
transport_idle_timeout_ (otherwise call report_option_value_error or
clamp/reject the value); ensure you touch the code that sets
transport_idle_timeout_ and the call to report_option_value_error to avoid any
out-of-bounds argv access and to enforce non-negative timeout semantics.

In `@TAO/tests/Transport_Idle_Timeout/Echo_i.cpp`:
- Around line 56-78: The non-nil server branch in Echo_i::ping skips verifying
cache_size_expected for the current ORB; update the logic so that after the
server->ping call (and any subsequent sleep_with_reactor), you also call check
(e.g., check("ping", cache_size(this->orb_), cache_size_expected)) to assert the
local cache size expectation, while still keeping the existing
check("ping_second", cache_size(this->orb_), cache_size_expected_in_server1) for
the second-server validation; ensure these checks run in the server != nil path
in function Echo_i::ping.

---

Nitpick comments:
In `@ACE/ace/Event_Handler_Handle_Timeout_Upcall.cpp`:
- Line 32: The local flag requires_reference_counting is assigned once and
should be made const for consistency: change the declaration "bool
requires_reference_counting = false;" in Event_Handler_Handle_Timeout_Upcall.cpp
to use a const-qualified type (e.g., bool const requires_reference_counting =
false;) and likewise update the similar declaration in the cancel_type function
(currently "int requires_reference_counting") to "int const
requires_reference_counting" so both variables are immutable and consistent with
the WFMO_Reactor changes.

In `@ACE/ace/WFMO_Reactor.cpp`:
- Around line 2065-2067: The complex_dispatch_handler uses an int named
reference_counting_required for the reference-counting check which is
inconsistent with simple_dispatch_handler; change the variable in
complex_dispatch_handler to match the pattern: declare a bool const
requires_reference_counting and set it by comparing
event_handler->reference_counting_policy().value() ==
ACE_Event_Handler::Reference_Counting_Policy::ENABLED (same expression used in
simple_dispatch_handler), update any subsequent uses of the old variable name to
the new name, and remove the int declaration to standardize naming and type
across dispatch handlers.

In `@TAO/docs/Options.html`:
- Around line 1393-1396: Update the documentation for the
-ORBTransportIdleTimeout option to explicitly state that a value of 0 disables
idle-timeout purging; modify the description for -ORBTransportIdleTimeout so it
still explains default behavior but adds a sentence like “A value of 0 disables
idle-timeout purging (connections are never closed based on idle timeout).” to
make the API contract unambiguous.

In `@TAO/tests/Transport_Idle_Timeout/client_multiple.cpp`:
- Line 297: The line initializing echo2 has a stray double semicolon:
"Test::Echo_var echo2 = Test::Echo::_narrow (obj2.in ());;". Remove the extra
trailing semicolon so the statement ends with a single semicolon; locate this in
the Test::Echo_var echo2 initialization (the _narrow call) and correct it.

In `@TAO/tests/Transport_Idle_Timeout/client.cpp`:
- Line 107: The ACE_Get_Opt option string passed to ACE_Get_Opt (ACE_Get_Opt
get_opts (argc, argv, ACE_TEXT ("k:l:t:n:d"))); includes an unused 'l:' option;
remove 'l:' from the string so it becomes ACE_TEXT("k:t:n:d") to match the
single-server client CLI and avoid confusion, and update any related
parsing/switch cases in the same scope (e.g., the ACE_Get_Opt instance and its
switch handling) to ensure no case for 'l' remains.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: da31ae77-183f-47cd-8097-47264daf121a

📥 Commits

Reviewing files that changed from the base of the PR and between 98e5a3f and 5dff3e1.

📒 Files selected for processing (28)

• ACE/ace/Event_Handler_Handle_Timeout_Upcall.cpp
• ACE/ace/WFMO_Reactor.cpp
• TAO/NEWS
• TAO/bin/tao_orb_tests.lst
• TAO/docs/Options.html
• TAO/tao/Cache_Entries_T.inl
• TAO/tao/Resource_Factory.h
• TAO/tao/Transport.cpp
• TAO/tao/Transport.h
• TAO/tao/Transport_Cache_Manager_T.cpp
• TAO/tao/Transport_Cache_Manager_T.h
• TAO/tao/Transport_Cache_Manager_T.inl
• TAO/tao/Transport_Connector.cpp
• TAO/tao/Transport_Timer.cpp
• TAO/tao/Transport_Timer.h
• TAO/tao/default_resource.cpp
• TAO/tao/default_resource.h
• TAO/tao/tao.mpc
• TAO/tests/Transport_Idle_Timeout/Echo_i.cpp
• TAO/tests/Transport_Idle_Timeout/Echo_i.h
• TAO/tests/Transport_Idle_Timeout/Idle_Transport_Timeout.mpc
• TAO/tests/Transport_Idle_Timeout/client.cpp
• TAO/tests/Transport_Idle_Timeout/client_multiple.cpp
• TAO/tests/Transport_Idle_Timeout/run_test.pl
• TAO/tests/Transport_Idle_Timeout/server.cpp
• TAO/tests/Transport_Idle_Timeout/svc.conf
• TAO/tests/Transport_Idle_Timeout/svc_disabled.conf
• TAO/tests/Transport_Idle_Timeout/test.idl

💤 Files with no reviewable changes (1)

• TAO/tao/Cache_Entries_T.inl