Issue 341741060: tryton-env: Reset less often the user sessions (Closed)
DescriptionPatch Set 1 #
Total comments: 18
Patch Set 2 : Fix remarks #
Total comments: 17
Patch Set 3 : Fix remarks #
Total comments: 7
Patch Set 4 : Fix remarks #
Total comments: 3
Patch Set 5 : Do not reset when last_reset is None #Patch Set 6 : Restore previous behaviour #
Total comments: 2
Patch Set 7 : Resetting session to it's write_date #
Total comments: 5
Patch Set 8 : Update to tip and fix remarks #
Total comments: 2
Patch Set 9 : Fix remarks #MessagesTotal messages: 51
flake8 OK URL: https://codereview.tryton.org/341741060
Sign in to reply to this message.
I think we need a changelog entry. I think it should talk about caching the user session.
Sign in to reply to this message.
I agree with pokoli, there should be a changelog entry. The commit description could include some words about the cache usage. https://codereview.tryton.org/341741060/diff/341761006/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/341761006/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:89: session.write_date or datetime.datetime.min) Why not just: session.Write_date or session.create_date https://codereview.tryton.org/341741060/diff/341761006/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:126: seconds=config.getint('session', 'timeout') // 6) I think it is better to have fixed precision like 1 minute. https://codereview.tryton.org/341741060/diff/341761006/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:127: timestamp = now - timeout no need to compute it if it is not used. https://codereview.tryton.org/341741060/diff/341761006/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:138: cls.write(sessions, {}) I think it will be good to set the cache to None.
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/341761006/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/341761006/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:89: session.write_date or datetime.datetime.min) On 2021/03/25 21:56:01, ced wrote: > Why not just: session.Write_date or session.create_date I know that it's shorter but it relies on the evaluation order and I prefer something that is semantically more obvious. https://codereview.tryton.org/341741060/diff/341761006/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:126: seconds=config.getint('session', 'timeout') // 6) On 2021/03/25 21:56:01, ced wrote: > I think it is better to have fixed precision like 1 minute. I still don't see why. A fixed limit could be bigger than the timeout while using a division (we can discuss about what the divisor should be) ensure it will always be lower. https://codereview.tryton.org/341741060/diff/341761006/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:127: timestamp = now - timeout On 2021/03/25 21:56:02, ced wrote: > no need to compute it if it is not used. Done. https://codereview.tryton.org/341741060/diff/341761006/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:138: cls.write(sessions, {}) On 2021/03/25 21:56:02, ced wrote: > I think it will be good to set the cache to None. Done.
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/341761006/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/341761006/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:89: session.write_date or datetime.datetime.min) On 2021/03/31 10:09:43, nicoe wrote: > On 2021/03/25 21:56:01, ced wrote: > > Why not just: session.Write_date or session.create_date > > I know that it's shorter but it relies on the evaluation order and I prefer > something that is semantically more obvious. We do that all over the place. Also you are relying on the same evaluation order for 'session.write_date or datetime.datetime.min'. So I do not see the point to call max. https://codereview.tryton.org/341741060/diff/341761006/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:126: seconds=config.getint('session', 'timeout') // 6) On 2021/03/31 10:09:43, nicoe wrote: > On 2021/03/25 21:56:01, ced wrote: > > I think it is better to have fixed precision like 1 minute. > > I still don't see why. It gives a more precise value of the last activity. > A fixed limit could be bigger than the timeout while using a division (we can > discuss about what the divisor should be) ensure it will always be lower. If the timeout is lower than the minute, just always write.
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/341741060
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/341761006/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/341761006/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:89: session.write_date or datetime.datetime.min) On 2021/03/31 10:19:33, ced wrote: > On 2021/03/31 10:09:43, nicoe wrote: > > On 2021/03/25 21:56:01, ced wrote: > > > Why not just: session.Write_date or session.create_date > > > > I know that it's shorter but it relies on the evaluation order and I prefer > > something that is semantically more obvious. > > We do that all over the place. I know, I don't like it. > Also you are relying on the same evaluation order for 'session.write_date or > datetime.datetime.min'. Indeed but it's to set a default value. > So I do not see the point to call max. You can see that you want the max, while the or does not care about which one is the biggest. https://codereview.tryton.org/341741060/diff/341761006/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:126: seconds=config.getint('session', 'timeout') // 6) On 2021/03/31 10:19:33, ced wrote: > On 2021/03/31 10:09:43, nicoe wrote: > > On 2021/03/25 21:56:01, ced wrote: > > > I think it is better to have fixed precision like 1 minute. > > > > I still don't see why. > > It gives a more precise value of the last activity. Which is not used anywhere or for anything. > > A fixed limit could be bigger than the timeout while using a division (we can > > discuss about what the divisor should be) ensure it will always be lower. > > If the timeout is lower than the minute, just always write. Which would add additional code just for this.
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/341761006/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/341761006/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:89: session.write_date or datetime.datetime.min) On 2021/03/31 12:04:54, nicoe wrote: > On 2021/03/31 10:19:33, ced wrote: > > On 2021/03/31 10:09:43, nicoe wrote: > > > On 2021/03/25 21:56:01, ced wrote: > > > > Why not just: session.Write_date or session.create_date > > > > > > I know that it's shorter but it relies on the evaluation order and I prefer > > > something that is semantically more obvious. > > > > We do that all over the place. > > I know, I don't like it. > > > Also you are relying on the same evaluation order for 'session.write_date or > > datetime.datetime.min'. > > Indeed but it's to set a default value. > > > So I do not see the point to call max. > > You can see that you want the max, while the or does not care about which one is > the biggest. One more reason that it is wrong because it does not have the same behavior as other places. https://codereview.tryton.org/341741060/diff/341761006/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:126: seconds=config.getint('session', 'timeout') // 6) On 2021/03/31 12:04:54, nicoe wrote: > On 2021/03/31 10:19:33, ced wrote: > > On 2021/03/31 10:09:43, nicoe wrote: > > > On 2021/03/25 21:56:01, ced wrote: > > > > I think it is better to have fixed precision like 1 minute. > > > > > > I still don't see why. > > > > It gives a more precise value of the last activity. > > Which is not used anywhere or for anything. For now but it will be interesting to have a last seen. > > > A fixed limit could be bigger than the timeout while using a division (we > can > > > discuss about what the divisor should be) ensure it will always be lower. > > > > If the timeout is lower than the minute, just always write. > > Which would add additional code just for this. Not more than your computation. reset_timeout = datetime.timedelta(seconds=min(config.getint('session', 'timeout'), 60))
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/341761006/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/341761006/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:89: session.write_date or datetime.datetime.min) On 2021/03/31 12:20:47, ced wrote: > On 2021/03/31 12:04:54, nicoe wrote: > > On 2021/03/31 10:19:33, ced wrote: > > > On 2021/03/31 10:09:43, nicoe wrote: > > > > On 2021/03/25 21:56:01, ced wrote: > > > > > Why not just: session.Write_date or session.create_date > > > > > > > > I know that it's shorter but it relies on the evaluation order and I > prefer > > > > something that is semantically more obvious. > > > > > > We do that all over the place. > > > > I know, I don't like it. > > > > > Also you are relying on the same evaluation order for 'session.write_date or > > > datetime.datetime.min'. > > > > Indeed but it's to set a default value. > > > > > So I do not see the point to call max. > > > > You can see that you want the max, while the or does not care about which one > is > > the biggest. > > One more reason that it is wrong because it does not have the same behavior as > other places. I don't understand: I want the biggest. https://codereview.tryton.org/341741060/diff/341761006/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:126: seconds=config.getint('session', 'timeout') // 6) On 2021/03/31 12:20:47, ced wrote: > On 2021/03/31 12:04:54, nicoe wrote: > > On 2021/03/31 10:19:33, ced wrote: > > > On 2021/03/31 10:09:43, nicoe wrote: > > > > On 2021/03/25 21:56:01, ced wrote: > > > > > I think it is better to have fixed precision like 1 minute. > > > > > > > > I still don't see why. > > > > > > It gives a more precise value of the last activity. > > > > Which is not used anywhere or for anything. > > For now but it will be interesting to have a last seen. Adding this concept to ir.session would be adding a functional meaning to something that is purely technical. Which is quite odd. > > > > A fixed limit could be bigger than the timeout while using a division (we > > can > > > > discuss about what the divisor should be) ensure it will always be lower. > > > > > > If the timeout is lower than the minute, just always write. > > > > Which would add additional code just for this. > > Not more than your computation. > > reset_timeout = datetime.timedelta(seconds=min(config.getint('session', > 'timeout'), 60)) Indeed
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/341761006/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/341761006/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:89: session.write_date or datetime.datetime.min) On 2021/03/31 17:58:45, nicoe wrote: > On 2021/03/31 12:20:47, ced wrote: > > On 2021/03/31 12:04:54, nicoe wrote: > > > On 2021/03/31 10:19:33, ced wrote: > > > > On 2021/03/31 10:09:43, nicoe wrote: > > > > > On 2021/03/25 21:56:01, ced wrote: > > > > > > Why not just: session.Write_date or session.create_date > > > > > > > > > > I know that it's shorter but it relies on the evaluation order and I > > prefer > > > > > something that is semantically more obvious. > > > > > > > > We do that all over the place. > > > > > > I know, I don't like it. > > > > > > > Also you are relying on the same evaluation order for 'session.write_date > or > > > > datetime.datetime.min'. > > > > > > Indeed but it's to set a default value. > > > > > > > So I do not see the point to call max. > > > > > > You can see that you want the max, while the or does not care about which > one > > is > > > the biggest. > > > > One more reason that it is wrong because it does not have the same behavior as > > other places. > > I don't understand: I want the biggest. Everywhere in Tryton (and especially in session) we consider that the last modification is write_date or create_date, no matter if the create_date is bigger due to time shift. This must be respected everywhere to have a consistent behavior. https://codereview.tryton.org/341741060/diff/341761006/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:126: seconds=config.getint('session', 'timeout') // 6) On 2021/03/31 17:58:45, nicoe wrote: > On 2021/03/31 12:20:47, ced wrote: > > On 2021/03/31 12:04:54, nicoe wrote: > > > On 2021/03/31 10:19:33, ced wrote: > > > > On 2021/03/31 10:09:43, nicoe wrote: > > > > > On 2021/03/25 21:56:01, ced wrote: > > > > > > I think it is better to have fixed precision like 1 minute. > > > > > > > > > > I still don't see why. > > > > > > > > It gives a more precise value of the last activity. > > > > > > Which is not used anywhere or for anything. > > > > For now but it will be interesting to have a last seen. > > Adding this concept to ir.session would be adding a functional meaning to > something that is purely technical. Which is quite odd. Such information is common for example we have it on discourse.
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:26: _session_timeout_cache = Cache('ir_session.keys', context=False) The Cache name and the attribute should be related. https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:89: session.write_date or datetime.datetime.min) It must be 'session.write_date or session.create_date' to have the same behavior as everywhere else. https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:126: seconds=config.getint('session', 'timeout') // 6) I still think it is better to have a fixed precision like 1 minute. https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:128: if not last_reset or (now - reset_timeout) > last_reset: I think it is better to test: last_reset is not None as it will detect invalid data. https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:139: cls._session_timeout_cache.set(key, None) I'm wondering if the reset should not be done on .write. This will be more modular.
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:26: _session_timeout_cache = Cache('ir_session.keys', context=False) On 2021/04/04 17:32:26, ced wrote: > The Cache name and the attribute should be related. Done. https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:89: session.write_date or datetime.datetime.min) On 2021/04/04 17:32:26, ced wrote: > It must be 'session.write_date or session.create_date' to have the same behavior > as everywhere else. Done. https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:126: seconds=config.getint('session', 'timeout') // 6) On 2021/04/04 17:32:27, ced wrote: > I still think it is better to have a fixed precision like 1 minute. We could always modify this part when ir.session is used from something else. https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:128: if not last_reset or (now - reset_timeout) > last_reset: On 2021/04/04 17:32:26, ced wrote: > I think it is better to test: last_reset is not None > as it will detect invalid data. Done. https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:139: cls._session_timeout_cache.set(key, None) On 2021/04/04 17:32:26, ced wrote: > I'm wondering if the reset should not be done on .write. > This will be more modular. Done.
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:126: seconds=config.getint('session', 'timeout') // 6) On 2021/04/09 12:51:57, nicoe wrote: > On 2021/04/04 17:32:27, ced wrote: > > I still think it is better to have a fixed precision like 1 minute. > > We could always modify this part when ir.session is used from something else. What is the point to not do what we know will be needed?
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/341741060
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:126: seconds=config.getint('session', 'timeout') // 6) On 2021/04/09 13:05:14, ced wrote: > On 2021/04/09 12:51:57, nicoe wrote: > > On 2021/04/04 17:32:27, ced wrote: > > > I still think it is better to have a fixed precision like 1 minute. > > > > We could always modify this part when ir.session is used from something else. > > What is the point to not do what we know will be needed? We don't know it. For now, it's only an idea and we don't know yet if it's needed.
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:126: seconds=config.getint('session', 'timeout') // 6) On 2021/04/09 17:01:26, nicoe wrote: > On 2021/04/09 13:05:14, ced wrote: > > On 2021/04/09 12:51:57, nicoe wrote: > > > On 2021/04/04 17:32:27, ced wrote: > > > > I still think it is better to have a fixed precision like 1 minute. > > > > > > We could always modify this part when ir.session is used from something > else. > > > > What is the point to not do what we know will be needed? > > We don't know it. For now, it's only an idea and we don't know yet if it's > needed. OK it is no more an idea: https://bugs.tryton.org/issue10261
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:126: seconds=config.getint('session', 'timeout') // 6) On 2021/04/09 17:56:48, ced wrote: > On 2021/04/09 17:01:26, nicoe wrote: > > On 2021/04/09 13:05:14, ced wrote: > > > On 2021/04/09 12:51:57, nicoe wrote: > > > > On 2021/04/04 17:32:27, ced wrote: > > > > > I still think it is better to have a fixed precision like 1 minute. > > > > > > > > We could always modify this part when ir.session is used from something > > else. > > > > > > What is the point to not do what we know will be needed? > > > > We don't know it. For now, it's only an idea and we don't know yet if it's > > needed. > > OK it is no more an idea: https://bugs.tryton.org/issue10261 Are you really going to implement something just to be right?
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:126: seconds=config.getint('session', 'timeout') // 6) On 2021/04/09 18:44:01, nicoe wrote: > On 2021/04/09 17:56:48, ced wrote: > > On 2021/04/09 17:01:26, nicoe wrote: > > > On 2021/04/09 13:05:14, ced wrote: > > > > On 2021/04/09 12:51:57, nicoe wrote: > > > > > On 2021/04/04 17:32:27, ced wrote: > > > > > > I still think it is better to have a fixed precision like 1 minute. > > > > > > > > > > We could always modify this part when ir.session is used from something > > > else. > > > > > > > > What is the point to not do what we know will be needed? > > > > > > We don't know it. For now, it's only an idea and we don't know yet if it's > > > needed. > > > > OK it is no more an idea: https://bugs.tryton.org/issue10261 > > Are you really going to implement something just to be right? It is not about being right. The questioning about the session last modification precision shows that we could have an interesting feature. But as you do not want to take it into account until it is implemented (in contrary to what we usually do), I implement it so the discussion is over.
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:126: seconds=config.getint('session', 'timeout') // 6) On 2021/04/09 19:21:39, ced wrote: > On 2021/04/09 18:44:01, nicoe wrote: > > On 2021/04/09 17:56:48, ced wrote: > > > On 2021/04/09 17:01:26, nicoe wrote: > > > > On 2021/04/09 13:05:14, ced wrote: > > > > > On 2021/04/09 12:51:57, nicoe wrote: > > > > > > On 2021/04/04 17:32:27, ced wrote: > > > > > > > I still think it is better to have a fixed precision like 1 minute. > > > > > > > > > > > > We could always modify this part when ir.session is used from > something > > > > else. > > > > > > > > > > What is the point to not do what we know will be needed? > > > > > > > > We don't know it. For now, it's only an idea and we don't know yet if it's > > > > needed. > > > > > > OK it is no more an idea: https://bugs.tryton.org/issue10261 > > > > Are you really going to implement something just to be right? > > It is not about being right. The questioning about the session last modification > precision shows that we could have an interesting feature. But as you do not > want to take it into account until it is implemented (in contrary to what we > usually do), I implement it so the discussion is over. I must admit I was temped to implement it also but didn't want to start a war :-) Both are interesting features. Thank you both for working on them and have a nice weekend.
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/340491002/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:126: seconds=config.getint('session', 'timeout') // 6) On 2021/04/09 19:21:39, ced wrote: > On 2021/04/09 18:44:01, nicoe wrote: > > On 2021/04/09 17:56:48, ced wrote: > > > On 2021/04/09 17:01:26, nicoe wrote: > > > > On 2021/04/09 13:05:14, ced wrote: > > > > > On 2021/04/09 12:51:57, nicoe wrote: > > > > > > On 2021/04/04 17:32:27, ced wrote: > > > > > > > I still think it is better to have a fixed precision like 1 minute. > > > > > > > > > > > > We could always modify this part when ir.session is used from > something > > > > else. > > > > > > > > > > What is the point to not do what we know will be needed? > > > > > > > > We don't know it. For now, it's only an idea and we don't know yet if it's > > > > needed. > > > > > > OK it is no more an idea: https://bugs.tryton.org/issue10261 > > > > Are you really going to implement something just to be right? > > It is not about being right. The questioning about the session last modification > precision shows that we could have an interesting feature. How is that an interesting feature? It just shows that a user has interacted with the server in some way but appart from that you get no info: you don't know what he did, you don't know which action he took, all you know is that he is still alive. You might as well take your phone and call him or use an email. > I implement it so the discussion is over. We can still discuss the feature which is according to me useless. It's not because we can do something that it should be done.
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/348161004/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/348161004/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:49: itertools.chain([records, values], args), 0, None, 2): Why not use the same pattern as everywhere else? for sessions in args[0:None:2]: … There is no point to define separate records, values. https://codereview.tryton.org/341741060/diff/348161004/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:103: cls._session_timeout_cache.set(key, last_reset) It sounds more logical that the cache is named: _session_last_reset https://codereview.tryton.org/341741060/diff/348161004/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:133: seconds=config.getint('session', 'timeout') // 6) Another reason for a fixed timeout of 1 minute is that by default this result in 50 seconds. Also with relative delay, on bigger timeout like 1h the user is loosing the last 10min of its authenticated period which he can perceive. The goal is to prevent write for the consecutive calls, there is no point to try to save for distant calls when the user was idle. It just creates annoying behavior for the user by requesting too soon to re-authenticate.
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/348161004/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/348161004/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:133: seconds=config.getint('session', 'timeout') // 6) On 2021/04/10 15:29:35, ced wrote: > Another reason for a fixed timeout of 1 minute is that by default this result in > 50 seconds. > Also with relative delay, on bigger timeout like 1h the user is loosing the last > 10min of its authenticated period which he can perceive. > > The goal is to prevent write for the consecutive calls, there is no point to try > to save for distant calls when the user was idle. It just creates annoying > behavior for the user by requesting too soon to re-authenticate. As last seen cannot really be implemented with this current mechanism. I guess we can go with a fraction of the timeout. But I'm wondering if 1/6 is a good number. The initial though is to use 1/4 but I'm wondering if it does not make more sense to use 1 scale lower and use 1/10.
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/348161004/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/348161004/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:49: itertools.chain([records, values], args), 0, None, 2): On 2021/04/10 15:29:35, ced wrote: > Why not use the same pattern as everywhere else? > > for sessions in args[0:None:2]: > … > > There is no point to define separate records, values. Done. https://codereview.tryton.org/341741060/diff/348161004/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:103: cls._session_timeout_cache.set(key, last_reset) On 2021/04/10 15:29:35, ced wrote: > It sounds more logical that the cache is named: _session_last_reset Done. https://codereview.tryton.org/341741060/diff/348161004/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:133: seconds=config.getint('session', 'timeout') // 6) On 2021/04/12 08:14:26, ced wrote: > On 2021/04/10 15:29:35, ced wrote: > > Another reason for a fixed timeout of 1 minute is that by default this result > in > > 50 seconds. > > Also with relative delay, on bigger timeout like 1h the user is loosing the > last > > 10min of its authenticated period which he can perceive. > > > > The goal is to prevent write for the consecutive calls, there is no point to > try > > to save for distant calls when the user was idle. It just creates annoying > > behavior for the user by requesting too soon to re-authenticate. > > As last seen cannot really be implemented with this current mechanism. I guess > we can go with a fraction of the timeout. > But I'm wondering if 1/6 is a good number. The initial though is to use 1/4 but > I'm wondering if it does not make more sense to use 1 scale lower and use 1/10. I used 6 as our system is based on multiple of 6 for the time which results also in some way to an order of difference but let's go for 10. Done.
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/345881003/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/345881003/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:133: if last_reset is None or (now - reset_timeout) > last_reset: for me if last_reset is None, we must not refresh the session as we do not know if it is continuous.
Sign in to reply to this message.
patch is not applicable URL: https://codereview.tryton.org/341741060
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/345881003/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/345881003/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:133: if last_reset is None or (now - reset_timeout) > last_reset: On 2021/04/12 17:37:39, ced wrote: > for me if last_reset is None, we must not refresh the session as we do not know > if it is continuous. Done.
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/341741060
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/345881003/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/345881003/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:133: if last_reset is None or (now - reset_timeout) > last_reset: On 2021/04/12 19:09:48, nicoe wrote: > On 2021/04/12 17:37:39, ced wrote: > > for me if last_reset is None, we must not refresh the session as we do not > know > > if it is continuous. > > Done. Indeed I was wrong. If last_reset is None it does not break the session mechanism as we still do the same search. So I guess it is safer to update with last_reset is None.
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/341741060
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/351691002/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/351691002/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:49: cls._session_last_reset.set(session.key, None) I think that setting it to None generate unnecessary session update on concurrent requests. Indeed I think it should be set with the write_date.
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/351691002/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/351691002/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:49: cls._session_last_reset.set(session.key, None) On 2021/04/13 17:00:21, ced wrote: > I think that setting it to None generate unnecessary session update on > concurrent requests. > Indeed I think it should be set with the write_date. It can be something like: cls._session_last_reset.set(session.key, session.write_date)
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/341741060
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/361871003/trytond/CHANGELOG File trytond/CHANGELOG (right): https://codereview.tryton.org/341741060/diff/361871003/trytond/CHANGELOG#newc... trytond/CHANGELOG:1: * Reset less often the user sessions Perhaps? Reset the user sessions less often
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/361871003/trytond/CHANGELOG File trytond/CHANGELOG (right): https://codereview.tryton.org/341741060/diff/361871003/trytond/CHANGELOG#newc... trytond/CHANGELOG:1: * Reset less often the user sessions On 2021/05/10 16:54:06, dave wrote: > Perhaps? > Reset the user sessions less often +1 https://codereview.tryton.org/341741060/diff/361871003/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/361871003/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:130: reset_timeout = datetime.timedelta( I think it will be better named: reset_interval
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/361871003/trytond/CHANGELOG File trytond/CHANGELOG (right): https://codereview.tryton.org/341741060/diff/361871003/trytond/CHANGELOG#newc... trytond/CHANGELOG:1: * Reset less often the user sessions On 2021/05/10 16:54:06, dave wrote: > Perhaps? > Reset the user sessions less often Done. https://codereview.tryton.org/341741060/diff/361871003/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/361871003/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:130: reset_timeout = datetime.timedelta( On 2021/06/22 23:10:21, ced wrote: > I think it will be better named: reset_interval Done.
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/341741060
Sign in to reply to this message.
https://codereview.tryton.org/341741060/diff/435481003/trytond/trytond/ir/ses... File trytond/trytond/ir/session.py (right): https://codereview.tryton.org/341741060/diff/435481003/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:18: _session_last_reset = Cache('ir_session.session_timeout', context=False) Why are the name not the same? https://codereview.tryton.org/341741060/diff/435481003/trytond/trytond/ir/ses... trytond/trytond/ir/session.py:123: seconds=config.getint('session', 'timeout') // 10) For performance the configuration should be retrieve only once. And maybe we could also compute only once the reset_interval.
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/341741060
Sign in to reply to this message.
On 2022/09/10 22:29:03, ced wrote: > LGTM ping
Sign in to reply to this message.
New changeset 38bd66006727 by Nicolas Évrard in branch 'default': Reset less often the user sessions https://hg.tryton.org/trytond/rev/38bd66006727
Sign in to reply to this message.
New changeset 0d577fecd64c by Nicolas Évrard in branch 'default': Reset less often the user sessions https://hg.tryton.org/tryton-env/rev/0d577fecd64c
Sign in to reply to this message.
|
