Disable inactivity check for DataStorm session connections

[pepone]

Fix #4618

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Calling disableInactivityCheck() on an existing session's connection is redundant. When p != _sessions.end(), the session was created via createOrGet(), which already called disableInactivityCheck() on line 117. Consider adding a check: if (p == _sessions.end()) { connection->disableInactivityCheck(); } to only disable the check for newly retrieved cached connections.

Suggested change

	connection->disableInactivityCheck();
	if (p == _sessions.end())
	{
	connection->disableInactivityCheck();
	}

[Copilot]

Potential null pointer dereference. According to the Ice API documentation, ice_getCachedConnection() can return nullptr. If no session exists (line 372) and lookup->ice_getCachedConnection() returns null, this will cause a crash. Add a null check before calling disableInactivityCheck() and connection->toString() on line 377.

[bernardnormier]

With this PR, we make 3 calls to disableInactivityCheck, always just before the 3 calls to getConnectionManager()->add(, so it would make sense to move this call to disableInactivity check to ConnectionManager::add.

Then, two of the calls to disableInactivityCheck are for incoming connections (apparently - come from dispatches), while the third is for an outgoing connection (comes from an API response callback).

I assume we register all connections with the ConnectionManager (incoming and outgoing) - at least all connections tied to sessions? If not, it would be simpler/clear to disable the InactivityCheck communicat-wide by setting the client and server InactivityTimeout properties.

[pepone]

With this PR, we make 3 calls to disableInactivityCheck, always just before the 3 calls to getConnectionManager()->add(, so it would make sense to move this call to disableInactivity check to ConnectionManager::add

Yes that would be simpler. Updated.

Then, two of the calls to disableInactivityCheck are for incoming connections (apparently - come from dispatches), while the third is for an outgoing connection (comes from an API response callback).

The dispatches can be over outgoing connections too, as DataStorm re-uses a connection to the peer if already established.

if not, it would be simpler/clear to disable the InactivityCheck communicat-wide by setting the client and server InactivityTimeout properties.

This will not work when the user provides the communicator.