Skip to content

Replace OSAllocatedUnfairLock with Mutex - #66

Merged
ladd merged 5 commits into
mainfrom
lvt-baggage-visibility
Jun 18, 2026
Merged

ladd merged 5 commits into
mainfrom
lvt-baggage-visibility

Conversation

@ladd

@ladd ladd commented Jun 17, 2026 •

Copy link
Copy Markdown

Summary

Migrates locking from os.OSAllocatedUnfairLock to Mutex from the Synchronization framework. This is a slight efficiency improvement (avoids an allocation), and wrapping the object is safer/clearer.

Where the lock guarded a single object, the Mutex now wraps that object directly:

  • Baggage → Mutex<TelemetryAttributes?> (also resolves the prior "stored property is mutable" Swift 6 warning)
  • Meter → Mutex<[Instrument]> (active instruments)
  • Counter / ObservableCounter / ObservableUpDownCounter / ObservableGauge / Histogram / ExponentialHistogram → Mutex wrapping the instrument's values
  • FlushTimer → Mutex<Bool> (suspended flag)
  • StableGuidSampler → Mutex<Data> (GUID); the withLockUnchecked set path became a normal withLock
  • ExampleReporter → static Mutex<Data?> (session GUID)

Span and Tracer guarded several heterogeneous fields (some read elsewhere without locking), so they keep a single mutex that also serializes those fields, preserving the prior single-lock semantics. Read-only computed accessors were added where the wrapped value was previously a directly-readable stored property, to avoid churning the exporter and test call sites.

Also adds public get accessors for span and attributes on Baggage, for better ergonomics in client code.

Test plan

  • swift build — clean (only the pre-existing Baggage.span non-Sendable warning remains)
  • swift test — all 92 tests pass
  • Two tests that mutated through .values.reset() were updated to snapshotAndReset(), since values is now a read-only snapshot

@jparise
@bachand

ladd and others added 2 commits June 17, 2026 16:58
Migrate locking from os.OSAllocatedUnfairLock to Mutex from the
Synchronization framework. Where the lock guarded a single object, the
Mutex now wraps that object directly: Baggage attributes, Meter's active
instruments, the metric instruments' values, FlushTimer's suspended
flag, the sampler GUID, and ExampleReporter's session GUID.

Span and Tracer guarded several heterogeneous fields (some read
elsewhere without locking), so they keep a single mutex that also
serializes those fields, preserving the prior single-lock semantics.
Read-only computed accessors were added where the wrapped value was
previously a directly-readable stored property, to avoid churning the
exporter and test call sites.

Also adds public get accessors for `span` and `attributes` on Baggage.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Span and Tracer guard several heterogeneous fields, so rather than
having the Mutex wrap one of them as a stand-in, use a plain Mutex<Void>
lock and keep the guarded fields as ordinary stored properties.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ladd
ladd marked this pull request as ready for review June 18, 2026 00:15

@bachand bachand left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM with a few comments. Thanks @ladd!

Comment thread Sources/NautilusTelemetry/Metrics/Meter.swift Outdated
Comment thread Sources/NautilusTelemetry/Tracing/Baggage.swift Outdated
Comment on lines +323 to +325
/// Serializes mutation of the guarded mutable state (`_attributes`, `events`,
/// `links`, `retireCallbacks`, `endTime`).
private let lock = Mutex<Void>(())

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What do you think about continuing to use OSAllocatedUnfairLock here and in other cases where the value is Void? I don't feel strongly, but the code did seem a bit cleaner without the _ ins. 🤔

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I would ideally put the entire state storage inside the Mutex wrapper. I opted to save that for future refactor work.

@ladd
ladd force-pushed the lvt-baggage-visibility branch from b33dc94 to 6322cb6 Compare June 18, 2026 16:06
ladd and others added 2 commits June 18, 2026 09:12
The test asserted exact handler-call counts against a free-running
repeating timer, so a fire that landed in the window between an
expectation completing and the next test step would break the equality
checks. The shared counter was also mutated on the timer queue and read
on the test thread.

Guard the counter with a lock, drain any in-flight handler via a sync
barrier on the (serial) timer queue after suspending, snapshot the count
at suspend instead of asserting an exact value, and use an expectation
(rather than == 2) to confirm the timer fires again after resume.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Guard the test's shared state with a Mutex from the Synchronization
framework, consistent with the rest of the codebase.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ladd
ladd merged commit d1fb6ea into main Jun 18, 2026
4 checks passed
@ladd
ladd deleted the lvt-baggage-visibility branch June 18, 2026 18:00
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.

2 participants