Digging into session issues I've found this keep-alive thing a little bit overcomplicated and unclear. The keep-alive setting handles two things: session keeping and new-mail checking. Some plugins also bind to keep-alive to do some recurring actions (e.g. to display calendar alarms, etc.).
I think we should split these functionalities. So keep-alive action should be used only for session keeping. In such case we need to execute keep-alive action only after some inactivity time (e.g. session_lifetime
- 1min [or 5%]). It means timer should be resetted on every reequest.
Also it means we don't need a separate config option, because the interval would be calculated from session_lifetime. For session_lifetime=0 there would be no keep-alive requests.
Another thing is recurring check for new-mail. This would work as is, but... see below.
Last but not least. We should provide some "global" recurring action which would work across all tasks so, plugins could bind to it. One "global" action to minimize session race-conditions when few plugins will create their own recurring requests on the same time. In mail task we could "bind" our check-recent action to this global action.
In my opinion this would be a much clearer solution and would give as some additional possibilities:
- possibility to disable recurring checks (for new mail etc.) at all,
- better overall performance because keep-alive out of mail task will be
executed less frequently
I think also, we could rename keep_alive option description ("Check for new messages on") into something like "Update system state (check for new mail, etc.) on", and move it to User Interface section, so we could use it as our global action interval.
ps. I'm not sure we should change draft auto-save interval in any way to fit into this. For simplicity would be nice to use the same global action and its interval.
On Tue, May 1, 2012 at 12:49 PM, A.L.E.C alec@alec.pl wrote:
Digging into session issues I've found this keep-alive thing a little bit overcomplicated and unclear. The keep-alive setting handles two things: session keeping and new-mail checking. Some plugins also bind to keep-alive to do some recurring actions (e.g. to display calendar alarms, etc.).
I think we should split these functionalities. So keep-alive action should be used only for session keeping. In such case we need to execute keep-alive action only after some inactivity time (e.g. session_lifetime
- 1min [or 5%]). It means timer should be resetted on every reequest.
Also it means we don't need a separate config option, because the interval would be calculated from session_lifetime. For session_lifetime=0 there would be no keep-alive requests.
Wasn't the whole keep-alive timer removed in change 6135 ?
I'm currently testing this changeset adapted to my 0.7.2 installation. Am logged parallel to a) Roundcube without 6134/6135 b) Roundcube with changes 6134/6135 So far no erros on both, but it could take some time until I get into the session problem.
Another thing is recurring check for new-mail. This would work as is, but... see below.
Last but not least. We should provide some "global" recurring action which would work across all tasks so, plugins could bind to it. One "global" action to minimize session race-conditions when few plugins will create their own recurring requests on the same time. In mail task we could "bind" our check-recent action to this global action.
In my opinion this would be a much clearer solution and would give as some additional possibilities:
- possibility to disable recurring checks (for new mail etc.) at all,
- better overall performance because keep-alive out of mail task will be
executed less frequently
I think also, we could rename keep_alive option description ("Check for new messages on") into something like "Update system state (check for new mail, etc.) on", and move it to User Interface section, so we could use it as our global action interval.
Agree!
By the way, since applying R6134 and 6135, the following entries are added every 5min to sessions log:
[01-May-2012 12:40:58 +0200]: Session auth check failed for tmuc32t92k18dhpn0brnjtcra6; timeslot = 2012-05-01 12:40:00 [01-May-2012 12:40:58 +0200]: Send new auth cookie for tmuc32t92k18dhpn0brnjtcra6: S53fabb9110e91180a0496334c87df7aac72d70d9 [01-May-2012 12:45:58 +0200]: Session auth check failed for tmuc32t92k18dhpn0brnjtcra6; timeslot = 2012-05-01 12:45:00 [01-May-2012 12:45:58 +0200]: Send new auth cookie for tmuc32t92k18dhpn0brnjtcra6: S961ba77407fec5c916f36782b63a4f0f8dc1395e [01-May-2012 12:50:58 +0200]: Session auth check failed for tmuc32t92k18dhpn0brnjtcra6; timeslot = 2012-05-01 12:50:00 [01-May-2012 12:50:58 +0200]: Send new auth cookie for tmuc32t92k18dhpn0brnjtcra6: Se02d5ff4dd57e85adabddb48842a1fccb557d923 [01-May-2012 12:55:58 +0200]: Session auth check failed for tmuc32t92k18dhpn0brnjtcra6; timeslot = 2012-05-01 12:55:00 [01-May-2012 12:55:58 +0200]: Send new auth cookie for tmuc32t92k18dhpn0brnjtcra6: S8ad4d623dbc54cdc3e9d103f22c4e85965f867f7 [01-May-2012 13:00:38 +0200]: Session auth check failed for tmuc32t92k18dhpn0brnjtcra6; timeslot = 2012-05-01 13:00:00 [01-May-2012 13:00:38 +0200]: Send new auth cookie for tmuc32t92k18dhpn0brnjtcra6: S37c35e2013651e30185cb6aa621db6cdc8233e69
Claudio Kuenzler wrote:
On Tue, May 1, 2012 at 12:49 PM, A.L.E.C <alec@alec.pl mailto:alec@alec.pl> wrote:
Digging into session issues I've found this keep-alive thing a little bit overcomplicated and unclear. The keep-alive setting handles two things: session keeping and new-mail checking. Some plugins also bind to keep-alive to do some recurring actions (e.g. to display calendar alarms, etc.). I think we should split these functionalities. So keep-alive action should be used only for session keeping. In such case we need to execute keep-alive action only after some inactivity time (e.g. session_lifetime - 1min [or 5%]). It means timer should be resetted on every reequest. Also it means we don't need a separate config option, because the interval would be calculated from session_lifetime. For session_lifetime=0 there would be no keep-alive requests.Wasn't the whole keep-alive timer removed in change 6135 ?
Nope, that wasn't removed. I removed the separate request_timeout timer but set a timeout to the jquery ajax api instead.
By the way, since applying R6134 and 6135, the following entries are added every 5min to sessions log:
[01-May-2012 12:40:58 +0200]: Session auth check failed for tmuc32t92k18dhpn0brnjtcra6; timeslot = 2012-05-01 12:40:00 [01-May-2012 12:40:58 +0200]: Send new auth cookie for tmuc32t92k18dhpn0brnjtcra6: S53fabb9110e91180a0496334c87df7aac72d70d9 [...]
That's correct and I assume to have log_session enabled in config. The auth cookie is valid for 5 minutes (session_lifetime / 2) and is then replaced with a new value. That process is logged here for debugging purposes. The whole session auth cookie is meant to be an additional protection against session hijacking because it changes during a session whereas the actual session cookie doesn't.
~Thomas
A.L.E.C wrote:
Digging into session issues I've found this keep-alive thing a little bit overcomplicated and unclear. The keep-alive setting handles two things: session keeping and new-mail checking. Some plugins also bind to keep-alive to do some recurring actions (e.g. to display calendar alarms, etc.).
Right. The keep-alive is replaced in mail by check-recent in order to not have two independently running requests which may cause race-conditions.
I think we should split these functionalities. So keep-alive action should be used only for session keeping. In such case we need to execute keep-alive action only after some inactivity time (e.g. session_lifetime
- 1min [or 5%]). It means timer should be resetted on every reequest.
Also it means we don't need a separate config option, because the interval would be calculated from session_lifetime. For session_lifetime=0 there would be no keep-alive requests.
I agree that two config options are not really necessary. But keep-alive has to run more frequently than just session_lifetime - 5% because it may fail due connection problems or the javascript timer simply doesn't fire (which I eventually see happening when the browser tab is not active).
Another thing is recurring check for new-mail. This would work as is, but... see below.
Last but not least. We should provide some "global" recurring action which would work across all tasks so, plugins could bind to it. One "global" action to minimize session race-conditions when few plugins will create their own recurring requests on the same time. In mail task we could "bind" our check-recent action to this global action.
That's a reasonable suggestion and would certainly make the code clearer.
In my opinion this would be a much clearer solution and would give as some additional possibilities:
- possibility to disable recurring checks (for new mail etc.) at all,
- better overall performance because keep-alive out of mail task will be
executed less frequently
I'm not sure if this would run less frequently. It actually has to run with the minimum interval of all recurring checks which need to be done.
I think also, we could rename keep_alive option description ("Check for new messages on") into something like "Update system state (check for new mail, etc.) on", and move it to User Interface section, so we could use it as our global action interval.
"Update system state" is something no user understand but "Check for new messages" certainly is. However, this is just a label but more important is that it doesn't set the keep-alive interval (which is to be derived from session_lifetime) but the actual check-recent inrerval.
I basically agree to you suggestions but let's see if the recent changes help avoiding the session timeouts before we add more major changes to it.
Regards, Thomas
On 05/01/2012 01:49 PM, Thomas Bruederli wrote:
In my opinion this would be a much clearer solution and would give as some additional possibilities:
- possibility to disable recurring checks (for new mail etc.) at all,
- better overall performance because keep-alive out of mail task will be
executed less frequently
I'm not sure if this would run less frequently. It actually has to run with the minimum interval of all recurring checks which need to be done.
The "global action" will be run probably with current interval, but the new keep-alive action will be run less frequently because it would depend only on session_lifetime.
On 05/01/2012 01:55 PM, A.L.E.C wrote:
I'm not sure if this would run less frequently. It actually has to run with the minimum interval of all recurring checks which need to be done.
The "global action" will be run probably with current interval, but the new keep-alive action will be run less frequently because it would depend only on session_lifetime.
... and its execution will be postponed on any user (or our global recurring) action.
A.L.E.C wrote:
On 05/01/2012 01:55 PM, A.L.E.C wrote:
I'm not sure if this would run less frequently. It actually has to run with the minimum interval of all recurring checks which need to be done.
The "global action" will be run probably with current interval, but the new keep-alive action will be run less frequently because it would depend only on session_lifetime.
... and its execution will be postponed on any user (or our global recurring) action.
OK, now I understood entirely what you suggested.
So the implementation would be something like:
on (<page load>) setTimeout(<send keep-alive>, session_lifetime / 2)
on (<send some ajax request>) clearTimeout(<send keep-alive>)
on (<get ajax response or error>) setTimeout(<send keep-alive>, session_lifetime / 2)
~Thomas
On 05/01/2012 02:05 PM, Thomas Bruederli wrote:
OK, now I understood entirely what you suggested.
So the implementation would be something like:
on (<page load>) setTimeout(<send keep-alive>, session_lifetime / 2)
on (<send some ajax request>) clearTimeout(<send keep-alive>)
we could handle also non-ajax requests in frames maybe.
on (<get ajax response or error>) setTimeout(<send keep-alive>, session_lifetime / 2)
Something like that, but with setInterval/clearInterval.
On 05/01/2012 12:49 PM, A.L.E.C wrote:
Digging into session issues I've found this keep-alive thing a little bit overcomplicated and unclear. The keep-alive setting handles two things: session keeping and new-mail checking. Some plugins also bind to keep-alive to do some recurring actions (e.g. to display calendar alarms, etc.).
The changes are already implemented in keep-alive branch which will be merged into master soon (before 0.9-beta). All plugins which were using 'keep-alive' hook for state updates should now use 'refresh' hook. If there are any plugins which implement their own recurring requests should also consider switching to 'refresh' request.
Note: Refreshing can be now disabled by the user (or admin).
-
A.L.E.C -
Claudio Kuenzler -
Thomas Bruederli