ice icon indicating copy to clipboard operation
ice copied to clipboard

Glacier2 SessionManager's getSessionTimeout can return incorrect value.

Open externl opened this issue 4 years ago • 4 comments

We currently have two properties:

  • Glacier2.SessionTimeout (used with SessionThead to reap sessions)
  • Glacier2.Client.ACM.Timeout (used with ACM)

Both of these can independently close idle sessions.

SessionManager's getSessionTimeout always returns the value from Glacier2.SessionTimeout (https://github.com/zeroc-ice/ice/blob/3.7/cpp/src/Glacier2/SessionRouterI.cpp#L675, https://github.com/zeroc-ice/ice/blob/3.7/cpp/src/Glacier2/SessionRouterI.cpp#L1006)

If SessionTimeout is set and if Glacier2.Client.ACM.Timeout is not set we update Glacier2.Client.ACM.Timeout to use Glacier2.SessionTimeout. Otherwise the Glacier2.Client.ACM.Timeout default is used to also close connections.

When Glacie2.SessionTimeout is not set (no SessionThead thread), connections will still be closed by Glacier2.Client.ACM.Timeout. getSessionTimeout will return 0, which is not really correct, as it indicates an infinite timeout. However, connection can still be closed by the ACM timeout.

One option would be to return the ACM timeout if SessionTimeout has not been set.

externl avatar Nov 21 '19 21:11 externl

Note that Router.ice also provides a getACMTimeout, so a Glacier2 router client can always retrieve the router's session timeout and the ACM timeout.

In Glacier2::Router:

   /**
     *
     * Get the value of the ACM timeout. Clients supporting connection
     * heartbeats can enable them instead of explicitly sending keep
     * alives requests.
     *
     * NOTE: This method is only available since Ice 3.6.
     *
     * @return The timeout (in seconds).
     *
     **/
    ["nonmutating", "cpp:const"] idempotent int getACMTimeout();

bernardnormier avatar Nov 22 '19 03:11 bernardnormier

Here the expected "correct" usage was to first call getACMTimeout catch OperationNotExistException and fallback to getSessionTimeout in case you are talking with and old router.

https://github.com/zeroc-ice/ice/blob/21ed0d4f353b532b39d6a349d07f906124f9d3b2/csharp/src/Glacier2/SessionHelper.cs#L229-L241

For 4.0 we can deprecate getSessionTimeout and refreshSession For 3.7 I think better keep it as is

pepone avatar Apr 20 '20 11:04 pepone

For 3.8, I think we should:

  1. keep the Slice definitions as-is and don't break on-the-wire compatibility

  2. cleanup the configuration and keep a single configuration property for everything, namely Glacier2.Client.ACM.Timeout. Note: the value of this property can also come from Ice.ACM.Timeout or Ice.ACM.Server.Timeout.

I propose to remove Glacier2.SessionTimeout and make both getACMTimeout & getSessionTimeout return the same ACM timeout.

bernardnormier avatar Sep 15 '23 14:09 bernardnormier

Per https://github.com/zeroc-ice/ice/issues/1505#issuecomment-1721563418, the remaining configuration would be Glacier2.Client.IdleTimeout and Glacier2.Client.InactivityTimeout. The corresponding Ice. properties provide default values.

getACMTimeout & getSessionTimeout return the idle timeout.

The inactivity timeout is not returned by any operation. Glacier2 sets Glacier2.Client.InactivityTimeout to infinite (-1) unless the application configures this property explicitly.

bernardnormier avatar Sep 15 '23 18:09 bernardnormier