please can we kill off USE_FROZEN_DICTS

[richvdh]

I have just spent an hour wondering why things work on debian at all, despite it only having frozendict 0.5 (which canonicaljson ought to conflict with), and the tests failing spectacularly.

It turns out that in our default config, we set USE_FROZEN_DICTS=False, which does make things work on debian, but also means that our UTs are testing something rather different to our production environment.

Please can we decide whether events should be frozendicts or not?

[richvdh]

the debian packaging now runs the tests, which means that this is a blocker for doing the next release.

[hawkowl]

What do we get by having them be frozendicts? Just ensuring we accidentally don't change it later?

[erikjohnston]

We used to have a lot of bugs where we'd end up changing the events either a) before they'd get persisted or b) while they were in the event cache. This is obviously problematic, especially given the signatures and hashes are then invalidated. Originally, we had frozendict enabled by default, but it turned out that its quite a performance hit.

I agree its sub-optimal to test different things than what's on production, but I think its important to ensure that we have some way of being confident that we're not accidentally editing events. It may be worth running tests with the option both on and off, but that starts to become quite a lot of test runs.

On Wed, Sep 26, 2018 at 11:04 AM Amber Brown notifications@github.com wrote:

What do we get by having them be frozendicts? Just ensuring we accidentally don't change it later?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/matrix-org/synapse/issues/3941#issuecomment-424659423, or mute the thread https://github.com/notifications/unsubscribe-auth/AICaWNMX_R6SZx7rSw_WGdZQTLT9w0__ks5ue1E5gaJpZM4W3E5T .

[richvdh]

Thanks for the insights, Erik. I wouldn't super object if the default (including for the unit tests) was USE_FROZEN_DICTS=False, and USE_FROZEN_DICTS was enabled for [some of?] the sytest runs. Thoughts?

[erikjohnston]

That could work.

The only alternative that I can think of is to figure out if there is a more efficient way of asserting that FrozenEvents can't be changed without having to rewrite them as frozendicts/tuples, so that the option can be removed entirely. One potential option is to rewrite all the places that we access the dict to use accessor functions, and never return mutable objects. Though this all might turn into a bit of a time sink.

On Wed, Sep 26, 2018 at 11:25 AM Richard van der Hoff < notifications@github.com> wrote:

Thanks for the insights, Erik. I wouldn't super object if the default (including for the unit tests) was USE_FROZEN_DICTS=False, and USE_FROZEN_DICTS was enabled for [some of?] the sytest runs. Thoughts?

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/matrix-org/synapse/issues/3941#issuecomment-424665098, or mute the thread https://github.com/notifications/unsubscribe-auth/AICaWBUbH4ZUIjTwVUB9GqktxsW-SEScks5ue1Y1gaJpZM4W3E5T .

[hawkowl]

Python 3.3 has a MappingProxyType, which is a wrapper around a mapping (dict) that is read only, which I think is faster than creating a new FrozenDict.

[hawkowl]

Course of action: make it configurable, add it to some of the sytest CI runs, and our unittest runs.

[hawkowl]

See PR: https://github.com/matrix-org/synapse/pull/3987

[richvdh]

Course of action: make it configurable, add it to some of the sytest CI runs, and our unittest runs.

We turned it off for the UT runs, to keep things the same in the UTs and production.

[richvdh]

https://github.com/matrix-org/sytest/pull/500 is the PR to turn on frozendicts for sytests.