Skip to content

Don't use Arc in Settings unnecessarily#873

Merged
max-sixty merged 2 commits intomitsuhiko:masterfrom
dstu:feature/settings_non_send_sync
Feb 4, 2026
Merged

Don't use Arc in Settings unnecessarily#873
max-sixty merged 2 commits intomitsuhiko:masterfrom
dstu:feature/settings_non_send_sync

Conversation

@dstu
Copy link
Copy Markdown
Contributor

@dstu dstu commented Feb 4, 2026

Per discussion in #871, it seems better for Settings to have an Rc internally instead of an Arc. This PR makes that change.

This passes make test check-msrv lint check-minver format-check && cargo run -p cargo-insta -- test.

Question for review: it would be simpler, and arguably better, if Settings just had fields instead of an inner Rc. This should leave the Settings interface as it is, since it uses simple interior mutation (fields would continue to be private with public accessors/mutators).

As-is, Settings has clone-on-write semantics, extra-cheap cloning (bumping a refcount copying fields), and extremely modest memory savings because newly constructed Settings objects share a set of immutable fields. But I'm not sure that the complexity imposed by having Settings wrap a pointer to an inner ActualSettings is justified. Is there a reason for using this pattern?

dstu added 2 commits February 3, 2026 16:11
`Settings` wrap around a set of configuration fields which are shared between
instances, with clone-on-write semantics. (All `Settings` objects share the same
inner set of fields. When a field's value changes, the fields are cloned
locally.)

This commit changes things so that `Settings::new` actually creates a new set of
fields. Fields are still shared between thread-local `CURRENT_SETTINGS`
instances, with the same clone-on-write semantics.

Switching from `Arc` to `Rc` like this makes it feasible to store
non-`Send`/`Sync` types in `Settings`, which is desirable per mitsuhiko#872.
@dstu dstu force-pushed the feature/settings_non_send_sync branch from 495c6dc to 2dfde9f Compare February 4, 2026 04:45
@max-sixty max-sixty merged commit f680868 into mitsuhiko:master Feb 4, 2026
15 checks passed
max-sixty added a commit that referenced this pull request Feb 5, 2026
## Summary

Follow-up to #873 ("Don't use `Arc` in `Settings` unnecessarily").

- Remove `Send + Sync` bounds from `Redaction::Dynamic` variant
- Remove `Send + Sync` bounds from `dynamic_redaction()` function
- Remove `Send + Sync` bounds from `Settings::add_dynamic_redaction()`
method

Since `Settings` are stored in thread-local storage and wrapped in `Rc`,
they can never cross thread boundaries. The `Send + Sync` bounds were
vestigial from when `Arc` was used for `DEFAULT_SETTINGS`.

This relaxes the API to allow non-`Send` closures in dynamic redactions,
which is now safe since those closures can never be sent to another
thread.

## Test plan

- [x] All tests pass with `cargo test --all-features`
- [x] No clippy warnings

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-authored-by: Claude <noreply@anthropic.com>
@dstu dstu deleted the feature/settings_non_send_sync branch March 26, 2026 21:38
@max-sixty max-sixty mentioned this pull request Mar 27, 2026
max-sixty added a commit that referenced this pull request Mar 27, 2026
## Summary

- Bump version to 1.47.0
- Update CHANGELOG

### Changes included in this release

- Add `Comparator` trait for customizing how snapshot values are
compared. #872 (@dstu)
- Sort sequences in `sort_maps` to fix non-deterministic `HashSet`
snapshots. #876
- Improve TOML serialization error message for unsupported types. #880
- Remove unnecessary `Send + Sync` bounds from `Redaction`, allowing
non-`Send` closures in dynamic redactions. #874
- Don't use `Arc` in `Settings` unnecessarily. #873 (@dstu)
- Upgrade `console` to 0.16 and MSRV to 1.66. #885
- Upgrade `toml-edit` to 0.25. #882 (@alexanderkjall)

> _This was written by Claude Code on behalf of max-sixty_

Co-authored-by: Claude <noreply@anthropic.com>
@john-h-kastner-aws
Copy link
Copy Markdown

FYI, this is a breaking change (per cargo semver-checks)

    Building insta v1.47.0 (current)
       Built [   0.636s] (current)
     Parsing insta v1.47.0 (current)
      Parsed [   0.009s] (current)
    Building insta v1.46.0 (baseline)
       Built [   4.836s] (baseline)
     Parsing insta v1.46.0 (baseline)
      Parsed [   0.008s] (baseline)
    Checking insta v1.46.0 -> v1.47.0 (minor change)

--- failure auto_trait_impl_removed: auto trait no longer implemented ---

Description:
A public type has stopped implementing one or more auto traits. This can break downstream code that depends on the traits being implemented.
        ref: https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/auto_trait_impl_removed.ron

Failed in:
     Checked [   0.006s] 196 checks: 194 pass, 2 fail, 0 warn, 56 skip
  type Redactions is no longer Send, in /tmp/insta-1.47/insta/src/settings.rs:26
  type Redactions is no longer Sync, in /tmp/insta-1.47/insta/src/settings.rs:26
  type SettingsBindDropGuard is no longer Sync, in /tmp/insta-1.47/insta/src/settings.rs:666
  type Settings is no longer Send, in /tmp/insta-1.47/insta/src/settings.rs:190
  type Settings is no longer Sync, in /tmp/insta-1.47/insta/src/settings.rs:190
  type Redaction is no longer Send, in /tmp/insta-1.47/insta/src/redaction.rs:53
  type Redaction is no longer Sync, in /tmp/insta-1.47/insta/src/redaction.rs:53

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/inherent_method_missing.ron

Failed in:
  Snapshot::matches, previously in file /home/jkastner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/insta-1.46.0/src/snapshot.rs:516
  Snapshot::matches_fully, previously in file /home/jkastner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/insta-1.46.0/src/snapshot.rs:525

     Summary semver requires new major version: 2 major and 0 minor checks failed
    Finished [   6.538s] insta

(discussion appears to be in #872)

@max-sixty
Copy link
Copy Markdown
Collaborator

is anyone affected by this? or it's just theoretical?

@john-h-kastner-aws
Copy link
Copy Markdown

There's a small break in our tests (cedar-policy/cedar-spec#917). We can work around it, but it's enough to block an automatic update.

@max-sixty
Copy link
Copy Markdown
Collaborator

ok. if you would like a reversion, happy to make it. sorry for the trouble

max-sixty added a commit to max-sixty/insta that referenced this pull request Mar 30, 2026
The Arc→Rc change (mitsuhiko#873) and Send+Sync removal (mitsuhiko#874) were
semver-breaking — they removed auto-trait impls from Settings,
Redactions, Redaction, and SettingsBindDropGuard. This broke
downstream crates like cedar-policy that depend on Send + Sync.

Reverts to Arc and restores Send + Sync bounds on Redaction and
dynamic_redaction(). Adds Send + Sync to the Comparator trait
(new in 1.47.0) for compatibility. Adds a compile-time assertion
to prevent future regressions.

TODOs mark these as candidates for Rc in the next breaking change.

Co-Authored-By: Claude <noreply@anthropic.com>
max-sixty added a commit that referenced this pull request Mar 30, 2026
The Arc→Rc change (#873) and Send+Sync removal (#874) were
semver-breaking — they removed auto-trait impls from `Settings`,
`Redactions`, `Redaction`, and `SettingsBindDropGuard`. This broke
downstream crates like
[cedar-policy](cedar-policy/cedar-spec#917)
that depend on `Send + Sync`.

This reverts to `Arc` and restores `Send + Sync` bounds on `Redaction`
and `dynamic_redaction()`. Also adds `Send + Sync` to the `Comparator`
trait (new in 1.47.0) since `Arc<ActualSettings>` requires it. A
compile-time assertion prevents future regressions.

TODOs mark the `Arc` usage and `Comparator` bounds as candidates for
removal in the next breaking change.

Thanks to @john-h-kastner-aws for reporting in #873 (comment).

> _This was written by Claude Code on behalf of @max-sixty_

Co-authored-by: Claude <noreply@anthropic.com>
@max-sixty
Copy link
Copy Markdown
Collaborator

@john-h-kastner-aws I have reverted. Sorry again for the trouble

@max-sixty
Copy link
Copy Markdown
Collaborator

@dstu this makes the comparator slightly heavier. But the backward-compat is important, and seems like folks were relying on it. Hope that's OK.

@dstu
Copy link
Copy Markdown
Contributor Author

dstu commented Mar 31, 2026

Thanks for the heads-up. This doesn't appear to affect the insta-image crate, so things look good from that end.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants