Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Disable frozen dicts by default #3987

Merged
merged 9 commits into from
Oct 2, 2018
Merged

Disable frozen dicts by default #3987

merged 9 commits into from
Oct 2, 2018

Conversation

hawkowl
Copy link
Contributor

@hawkowl hawkowl commented Oct 1, 2018

No description provided.

richvdh
richvdh previously requested changes Oct 1, 2018
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make synapse.events.USE_FROZEN_DICTS default to False, except where it is overridden by the config?

richvdh
richvdh previously requested changes Oct 2, 2018
synapse/events/__init__.py Outdated Show resolved Hide resolved
synapse/events/__init__.py Outdated Show resolved Hide resolved
# NOTE: This is overridden by the configuration by the Synapse worker apps, but
# for the sake of tests, it is set here while it cannot be configured on the
# homeserver object itself.
USE_FROZEN_DICTS = strtobool(os.environ.get("SYNAPSE_USE_FROZEN_DICTS", "0"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there an intention to set this for the tests somewhere?

@@ -136,6 +136,8 @@ def default_config(name):
config.rc_messages_per_second = 10000
config.rc_message_burst_count = 10000

config.use_frozen_dicts = False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if this is actually used (since afaict use_frozen_dicts is only read by code which is never run under the UTs), and if not I would probably choose to omit this. But I'm not that bothered and this needs merging.

@hawkowl hawkowl merged commit 7232917 into develop Oct 2, 2018
@hawkowl hawkowl deleted the hawkowl/frozendicts branch October 2, 2018 15:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants