Issue 11399

Title
Modules manipulating the domain and states in their setup should invalidate the depends cached properties
Priority
bug
Status
chatting
Nosy list
ced, nicoe
Assigned to
Keywords

Created on 2022-04-10.18:11:55 by nicoe, last changed 4 months ago by nicoe.

Messages

Author: [hidden] (nicoe) Tryton committer
Date: 2022-04-11.15:38:26
* Cédric Krier  [2022-04-11 12:56 +0200]: 
>
>Cédric Krier <cedric.krier@b2ck.com> added the comment:
>
>On 2022-04-11 12:38, Nicolas Évrard wrote:
>> >Cédric Krier <cedric.krier@b2ck.com> added the comment:
>> >
>> >`invalidate_cached_property` was only added in version 1.0.0 of werkzeug but I
>> >think it is fine if for 6.4 we start requiring at least this version.
>>
>> But it was removed in version 2.1.
>
>I see no problem:
>
>if invalidate_cached_property:
>    invalidate_cached_property(...)
>else:
>    del ...

Of course it's doable.

>> I think it's more clever to vendor cached_property with a support for del and
>> remove it once we drop python3.7.
>
>I do not see the point to maintain it as we already have it in werkzeug.

Abstract away the inner stuffs of werkzeug.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2022-04-11.12:56:19
On 2022-04-11 12:38, Nicolas Évrard wrote:
> >Cédric Krier <cedric.krier@b2ck.com> added the comment:
> >
> >`invalidate_cached_property` was only added in version 1.0.0 of werkzeug but I
> >think it is fine if for 6.4 we start requiring at least this version.
> 
> But it was removed in version 2.1.

I see no problem:

if invalidate_cached_property:
    invalidate_cached_property(...)
else:
    del ...

> I think it's more clever to vendor cached_property with a support for del and
> remove it once we drop python3.7.

I do not see the point to maintain it as we already have it in werkzeug.
Author: [hidden] (nicoe) Tryton committer
Date: 2022-04-11.12:38:34
* Cédric Krier  [2022-04-11 11:02 +0200]: 
>
>Cédric Krier <cedric.krier@b2ck.com> added the comment:
>
>`invalidate_cached_property` was only added in version 1.0.0 of werkzeug but I
>think it is fine if for 6.4 we start requiring at least this version.

But it was removed in version 2.1.

I think it's more clever to vendor cached_property with a support for del and
remove it once we drop python3.7.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2022-04-11.11:02:38

invalidate_cached_property was only added in version 1.0.0 of werkzeug but I think it is fine if for 6.4 we start requiring at least this version.

Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2022-04-11.10:51:05

So we need to clear the cache of the depends in __post_setup__.
For stdlib cached_property we just use del and for werkzeug version we try first if invalidate_cached_property exists and use it otherwise we use del.

Author: [hidden] (nicoe) Tryton committer
Date: 2022-04-11.10:08:17
* Cédric Krier  [2022-04-10 23:09 +0200]: 
>
>Cédric Krier <cedric.krier@b2ck.com> added the comment:
>
>On 2022-04-10 20:21, Nicolas Évrard wrote:
>> >When a new module is activated, the models are setup again and every
>> >field is deepcopied. So normally the original field should not any
>> >cached properties.
>>
>> Indeed I forgot about the deepcopy, I guess we can close then.
>
>I think we must check that a deepcopy does not copy also the cached of
>the property.

I just checked and indeed deepcopy also copies the cache.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2022-04-10.23:09:37
On 2022-04-10 20:21, Nicolas Évrard wrote:
> >When a new module is activated, the models are setup again and every
> >field is deepcopied. So normally the original field should not any
> >cached properties.
> 
> Indeed I forgot about the deepcopy, I guess we can close then.

I think we must check that a deepcopy does not copy also the cached of
the property.
Author: [hidden] (nicoe) Tryton committer
Date: 2022-04-10.20:21:52
* Cédric Krier  [2022-04-10 19:59 +0200]: 
>
>Cédric Krier <cedric.krier@b2ck.com> added the comment:
>
>On 2022-04-10 19:27, Nicolas Évrard wrote:
>> >Cédric Krier <cedric.krier@b2ck.com> added the comment:
>> >
>> >As we discussed there is no valid case where the depend's should be computed
>> >in the `__setup__`.
>>
>> I am not talking about that.
>>
>> The scenario is:
>>
>>      - someone has some modules installed. The different depends properties are
>>        cached
>>      - someone activate a new module that modifies the states of an existing
>>        field (eg adding some readonly conditions) adding some PYSON expression.
>>
>> As the properties are cached the new fields might not be found in the depends
>> properties.
>
>When a new module is activated, the models are setup again and every
>field is deepcopied. So normally the original field should not any
>cached properties.

Indeed I forgot about the deepcopy, I guess we can close then.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2022-04-10.19:59:00
On 2022-04-10 19:27, Nicolas Évrard wrote:
> >Cédric Krier <cedric.krier@b2ck.com> added the comment:
> >
> >As we discussed there is no valid case where the depend's should be computed
> >in the `__setup__`.
> 
> I am not talking about that.
> 
> The scenario is:
> 
>      - someone has some modules installed. The different depends properties are
>        cached
>      - someone activate a new module that modifies the states of an existing
>        field (eg adding some readonly conditions) adding some PYSON expression.
> 
> As the properties are cached the new fields might not be found in the depends
> properties.

When a new module is activated, the models are setup again and every
field is deepcopied. So normally the original field should not any
cached properties.

> >We can for case where it is support clear the properties "just in case".
> 
> What's your proposition to clear on old versions of werkzeug?

I do not want to clear. Indeed I see the clearing as a safe guard in
case someone write bad code. But it will even be better to have an
assert if something was cleared.
Author: [hidden] (nicoe) Tryton committer
Date: 2022-04-10.19:27:07
* Cédric Krier  [2022-04-10 18:16 +0200]: 
>
>Cédric Krier <cedric.krier@b2ck.com> added the comment:
>
>As we discussed there is no valid case where the depend's should be computed
>in the `__setup__`.

I am not talking about that.

The scenario is:

     - someone has some modules installed. The different depends properties are
       cached
     - someone activate a new module that modifies the states of an existing
       field (eg adding some readonly conditions) adding some PYSON expression.

As the properties are cached the new fields might not be found in the depends
properties.

>We can for case where it is support clear the properties "just in case".

What's your proposition to clear on old versions of werkzeug?
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2022-04-10.18:16:34

As we discussed there is no valid case where the depend's should be computed in the __setup__.

We can for case where it is support clear the properties "just in case".

Author: [hidden] (nicoe) Tryton committer
Date: 2022-04-10.18:11:55

I realized that a problem with the cache invalidation could occur if a module manipulates the domain / states or whatever property of field given the fact that the depends properties computed from those are cached and not invalidated.

In python 3.8, it's not difficult to invalidate those: simply calling del field.readonly_depends will invalidate the cache. But for python3.7 we're using the werkzeug implementation and it only supports using del on werkzeug >= 2.0. For earlier version the idea I have is to use the set it with the _missing value but it's quite ugly and depends on an implementation detail of werkzeug's cached_property.

(A very simple fix would be to turn those in simple properties and live with the performance penalty until we drop python3.7).

History
Date User Action Args
2022-04-11 15:38:26nicoesetmessages: + msg75596
2022-04-11 12:56:19cedsetmessages: + msg75591
2022-04-11 12:38:34nicoesetmessages: + msg75586
2022-04-11 11:02:38cedsetmessages: + msg75584
2022-04-11 10:51:05cedsetmessages: + msg75583
2022-04-11 10:08:17nicoesetmessages: + msg75582
2022-04-10 23:09:37cedsetmessages: + msg75580
2022-04-10 20:21:52nicoesetmessages: + msg75577
2022-04-10 19:59:01cedsetmessages: + msg75576
2022-04-10 19:27:08nicoesetmessages: + msg75568

Showing 10 items. Show all history (warning: this could be VERY long)