fix: replace ReadWriteLock with NSLock for consistent thread-safety by Kilo-Loco · Pull Request #476 · PostHog/posthog-ios

Kilo-Loco · 2026-02-17T00:32:38Z

💡 Motivation and Context

The @ReadWriteLock property wrapper provides a false sense of thread-safety for collection types because the lock is acquired and released between separate operations, allowing the underlying collection to change between a check and a mutation.

💚 How did you test it?

Added concurrency tests covering concurrent adds, deletes, and mixed operations.

📝 Checklist

I reviewed the submitted code.
I added tests to verify the changes.
I updated the docs if needed.
No breaking change or entry added to the changelog.

marandaneto · 2026-02-17T12:21:09Z

        do {
            let filename = "\(Date().timeIntervalSince1970)"
            try contents.write(to: queue.appendingPathComponent(filename))
-            items.append(filename)


pop method is only called when PostHogQueue acquires isFlushingLock
PostHogQueue calls PostHogFileBackedQueue add and delete in the same method
we also call peek/clear from PostHogQueue
maybe a single lock for all those operations within the PostHogQueue would be better?
i'll let @ioannisj figure this out
but i do see some possible race conditions, good catch @Kilo-Loco

Pushed additional concurrency tests that expose a race condition in the queue. when multiple threads call add() simultaneously, they can generate identical filenames since Date().timeIntervalSince1970 and file operations are not atomic.

To clarify, this was always there and not added with this PR, but since we are working on this we should address it.

So one operation can overwrite the file of another add operation which can lead to data loss, if my rationale is correct here

ioannisj · 2026-02-18T07:29:20Z

Hey @Kilo-Loco, thank you for this. I think what we need to do is remove the dependance on @ReadWriteLock property wrapper since it's indeed problematic and provides a false sense of protection against collection types specifically. So a simple NSLock around items here could potentially be safer and communicate intent better

Having said that, I can only see this as problematic since the rest of the operations should be protected by @ReadWriteLock already:

if items.isEmpty { return }      // lock acquired, check, lock released
let removed = items.remove(at: index)  // lock acquired again, but items may have changed!

Kilo-Loco · 2026-02-19T01:41:30Z

Got it. I can remove the dependencies on @ReadWriteLock here, but now I'm wondering if the property wrapper should be removed altogether 🤔 I'm only seeing it be used here and in PostHogContext so maybe we can change both places for consistency across the codebase. I can scope the work in an issue to properly document the changes or just make the changes and update the PR body. Let me know the preferred path forward for the team.

ioannisj · 2026-02-20T10:13:40Z

Got it. I can remove the dependencies on @ReadWriteLock here, but now I'm wondering if the property wrapper should be removed altogether 🤔 I'm only seeing it be used here and in PostHogContext so maybe we can change both places for consistency across the codebase. I can scope the work in an issue to properly document the changes or just make the changes and update the PR body. Let me know the preferred path forward for the team.

Hey @Kilo-Loco I think we can remove ReadWriteLock all-together, agreed. It's better we keep locking consistent across the code and this is the only one that's sticking out. I'd say this can be done in this PR, just update title and body. We just migrated to changeset based release process so make sure to merge main

- Remove @ReadWriteLock property wrapper in favor of explicit NSLock - Update PostHogFileBackedQueue to use NSLock for items array - Update PostHogContext to use NSLock for screenSize - Delete ReadWriteLock.swift (no longer used) This provides clearer intent and consistency with the rest of the codebase.

Kilo-Loco · 2026-02-22T20:54:24Z

Removed @ReadWriteLock property wrapper dependency
Replaced with explicit NSLock for clearer intent
Merged main
Tests passing

Let me know if anything needs adjusting.

ioannisj

LG, left a nit comment
Let's add a changeset and label this PR and we are good to go I think

ioannisj · 2026-02-25T08:20:18Z

+        screenSizeLock.lock()
+        let currentScreenSize = screenSize
+        screenSizeLock.unlock()


nit: prefer .withLock which has a generic return type. Commenting just here but it can be replaced in multiple places

Suggested change

screenSizeLock.lock()

let currentScreenSize = screenSize

screenSizeLock.unlock()

let currentScreenSize = screenSizeLock.withLock { screenSize }

ioannisj · 2026-02-25T08:22:48Z

We switched to changeset based releases. So merge main and pnpm changeset + label this PR with release instead (details in RELEASING.md)

…-thread-safety

- Replace manual lock()/unlock() with .withLock closures - Add changeset for release

Kilo-Loco · 2026-02-26T04:04:36Z

Updated call sites to use .withLock block
Add changeset file

Ready to be labeled with release if everything else looks good 👍🏽

…ue-thread-safety # Conflicts: # CHANGELOG.md

ioannisj · 2026-03-12T10:10:57Z

@marandaneto I'll need your eyes on 2a443ab please. Addressing this comment here. This should be backward compatible with old filenames so no migration needed

marandaneto · 2026-03-12T13:12:47Z

    func add(_ contents: Data) {
        do {
-            let filename = "\(Date().timeIntervalSince1970)"
+            let filename = "\(Date().timeIntervalSince1970)-\(UUID.v7().uuidString)"


can we use only (UUID.v7().uuidString instead?
uuid v7 already has a timestamp in its string
you can also list files from a directory and sort by timestamp from its metadata (we do that on android btw)

The idea here was to be compatible when mixed with old file names. v7 is sortable which is very helpful, but I'll have to think how to handle the mix of UUIDv7 and old timestamps. I'll think about it

you can also list files from a directory and sort by timestamp from its metadata (we do that on android btw)

yeah probably this is the way

i just mentioned above how we do this for android
this should be compatible with whatever name the files are
see example https://github.com/PostHog/posthog-android/blob/65700f9159a789861b35dac1f936f76bb0683807/posthog/src/main/java/com/posthog/internal/PostHogQueue.kt#L417-L422

timeIntervalSince1970-v7 is also fine, not a blocker, just not needed because the approach above would solve the sorting

Okay implemented by modification date c33a357 even though API is not as elegant as Android

marandaneto · 2026-03-12T13:13:38Z

@marandaneto I'll need your eyes on 2a443ab please. Addressing this comment here. This should be backward compatible with old filenames so no migration needed

replied

Kilo-Loco requested a review from a team as a code owner February 17, 2026 00:32

Kilo-Loco force-pushed the fix/file-backed-queue-thread-safety branch from 043ca0b to d6a6c47 Compare February 17, 2026 00:38

fix: thread-safe array operations in FileBackedQueue

88e6244

Kilo-Loco force-pushed the fix/file-backed-queue-thread-safety branch from d6a6c47 to 88e6244 Compare February 17, 2026 00:38

chore: add changelog entry for thread-safety fix

b14ee20

marandaneto reviewed Feb 17, 2026

View reviewed changes

marandaneto requested a review from ioannisj February 17, 2026 12:21

Kilo-Loco changed the title ~~fix: use atomic mutate() for thread-safe array operations in FileBackedQueue~~ fix: replace ReadWriteLock with NSLock for consistent thread-safety Feb 21, 2026

Merge upstream/main into fix/file-backed-queue-thread-safety

10d65c2

Kilo-Loco force-pushed the fix/file-backed-queue-thread-safety branch from b838ce8 to 10d65c2 Compare February 22, 2026 20:31

Kilo-Loco added 2 commits February 23, 2026 20:51

fix: remove ReadWriteLock.swift from Xcode project file

5c2240e

fix: remove trailing whitespace

edbb965

ioannisj requested changes Feb 25, 2026

View reviewed changes

Kilo-Loco added 2 commits February 25, 2026 19:47

Merge remote-tracking branch 'origin/main' into fix/file-backed-queue…

1bfca64

…-thread-safety

refactor: use .withLock for cleaner locking pattern

f9bd795

- Replace manual lock()/unlock() with .withLock closures - Add changeset for release

Kilo-Loco and others added 7 commits February 27, 2026 04:59

Merge remote-tracking branch 'upstream/main' into fix/file-backed-que…

5c1aafa

…ue-thread-safety # Conflicts: # CHANGELOG.md

chore: update pnmp to proper version

96036d1

Merge branch 'main' into fix/file-backed-queue-thread-safety

81cb84f

feat: add more tests

25bd65c

fix: format

6f12aa4

fix: add uuid to resolve timestamp conflicts

2a443ab

fix: format

da7bef8

marandaneto reviewed Mar 12, 2026

View reviewed changes

ioannisj added 3 commits March 13, 2026 13:19

fix: sort queue files by modification date

c33a357

Merge branch 'main' into fix/file-backed-queue-thread-safety

26e50ac

Merge branch 'main' into fix/file-backed-queue-thread-safety

374f28e

ioannisj approved these changes Mar 18, 2026

View reviewed changes

ioannisj added release and removed release labels Mar 18, 2026

ioannisj merged commit 061cb44 into PostHog:main Mar 18, 2026
28 of 30 checks passed

Uh oh!

Conversation

Kilo-Loco commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ioannisj commented Feb 18, 2026

Uh oh!

Kilo-Loco commented Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ioannisj commented Feb 20, 2026

Uh oh!

Kilo-Loco commented Feb 22, 2026

Uh oh!

ioannisj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kilo-Loco commented Feb 26, 2026

Uh oh!

ioannisj commented Mar 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ioannisj Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marandaneto commented Mar 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Kilo-Loco commented Feb 17, 2026 •

edited

Loading

Kilo-Loco commented Feb 19, 2026 •

edited

Loading

ioannisj Mar 13, 2026 •

edited

Loading