zeroc-ice / ice

All-in-one solution for creating networked applications with RPC, pub/sub, server deployment, and more.
https://zeroc.com
GNU General Public License v2.0
2.04k stars 593 forks source link

Glacier2 SessionManager's getSessionTimeout can return incorrect value. #642

Closed externl closed 6 months ago

externl commented 4 years ago

We currently have two properties:

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.

bernardnormier commented 4 years ago

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();
pepone commented 4 years ago

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

bernardnormier commented 1 year ago

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 commented 1 year ago

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 commented 6 months ago

This was fixed by earlier PRs.