Skip to content

Add warning about known problems when configuring use_frozen_dicts#19711

Merged
MadLittleMods merged 5 commits intodevelopfrom
madlittlemods/known-bad-use-frozen-dicts
Apr 24, 2026
Merged

Add warning about known problems when configuring use_frozen_dicts#19711
MadLittleMods merged 5 commits intodevelopfrom
madlittlemods/known-bad-use-frozen-dicts

Conversation

@MadLittleMods
Copy link
Copy Markdown
Contributor

@MadLittleMods MadLittleMods commented Apr 20, 2026

Add warning about known problems when configuring use_frozen_dicts

As a follow-up, we should consider removing this config option altogether. It's "expensive" and claims to "prevent bugs" but actually introduces a whole new class of bugs. It could be re-introduced with a more holistic solution to the typing. Or a completely new approach (safe mode that blows up when someone mutates the event content, always make deep clones when handing out references, etc)

Dev notes

The use_frozen_dict config option was there since inception but was only recently documented for completeness sake.

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Code style is correct (run the linters)

Comment on lines +99 to +100
> ⚠️ **Warning** – This option is known to introduce a new class of [comparison
bugs](https://github.com/element-hq/synapse/issues/18117) in Synapse.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the same warning format as other places (we're not that consistent though)

> ⚠️ **Warning** – The additional consequences of replacing
[`macaroon_secret_key`](#macaroon_secret_key) will apply in case it
delegates to `registration_shared_secret`.

> ⚠️ **Warning** – Replacing an existing `macaroon_secret_key` with a new
one will lead to invalidation of access tokens for all guest users. It
will also break unsubscribe links in emails sent before the change. An
unlucky user might encounter a broken SSO login flow and would have to
start again.

@MadLittleMods MadLittleMods marked this pull request as ready for review April 20, 2026 17:11
@MadLittleMods MadLittleMods requested a review from a team as a code owner April 20, 2026 17:11
*(boolean)* Determines whether we should freeze the internal dict object in `FrozenEvent`. Freezing prevents bugs where we accidentally share e.g. signature dicts. However, freezing a dict is expensive. Defaults to `false`.
*(boolean)* Determines whether we should freeze the internal dict object in `FrozenEvent`. Freezing prevents bugs where we accidentally share e.g. signature dicts. However, freezing a dict is expensive.

> ⚠️ **Warning** – This option is known to introduce a new class of [comparison bugs](https://github.com/element-hq/synapse/issues/18117) in Synapse.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this implementation is buggy, wouldn't it just be better to remove it, or at least disabled it unconditionally until it works? Seems ill advised to me to leave options in that actively break software

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed 👍 PR description suggests a follow-up to remove.

I'd rather document this now than be held up by a refactor that may never happen or is a more controversial change in terms of getting it across the line. It also makes the follow-up PR more clear as the reasoning is contained within.

@MadLittleMods MadLittleMods merged commit 22e1643 into develop Apr 24, 2026
50 checks passed
@MadLittleMods MadLittleMods deleted the madlittlemods/known-bad-use-frozen-dicts branch April 24, 2026 17:00
@MadLittleMods
Copy link
Copy Markdown
Contributor Author

Thanks for the review @frebib and @erikjohnston 🐬

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants