Skip to content

Fix data race on SessionDataTask.started and tighten concurrency tests#2549

Open
onevcat wants to merge 1 commit into
masterfrom
fix-force-cancel-race
Open

Fix data race on SessionDataTask.started and tighten concurrency tests#2549
onevcat wants to merge 1 commit into
masterfrom
fix-force-cancel-race

Conversation

@onevcat

@onevcat onevcat commented Jul 5, 2026

Copy link
Copy Markdown
Owner

Summary

Follow-up to #2539 (thanks @devzahirul), which fixed the unlocked callbacksStore iteration in forceCancel() and is now merged.

  • Fix the same class of race on started, found while reviewing Fix data race in SessionDataTask.forceCancel() #2539: resume() wrote the flag unsynchronized while ImageDownloader.startDownloadTask reads it from other threads. It is now guarded by the existing lock, with resume() performing an atomic check-and-set and calling task.resume() outside the lock.
  • Condense the forceCancel comment from Fix data race in SessionDataTask.forceCancel() #2539 to the two load-bearing facts (cross-thread mutation; cancel(token:) re-acquires the non-recurrent lock).
  • Tighten the concurrency tests: instead of adding more single-purpose stress tests, add one combined testSessionDataTaskConcurrentAccessIsThreadSafe that hammers all mutating and reading surfaces of SessionDataTask concurrently (addCallback, forceCancel, resume, started, containsCallbacks, didReceiveData, mutableDataCount, completeAndRemoveAllCallbacks). The focused Fix data race in SessionDataTask.forceCancel() #2539 regression test remains as-is.

Verification

Before the fix, the started race is reported deterministically by Thread Sanitizer (process crashes):

WARNING: ThreadSanitizer: data race
  Write of size 1 by thread T3:
    #0 Kingfisher.SessionDataTask.started.setter

After the fix, both the combined test and the #2539 regression test run clean under TSan:

xcodebuild test -project Kingfisher.xcodeproj -scheme Kingfisher \
  -destination 'platform=macOS' -enableThreadSanitizer YES \
  -only-testing:KingfisherTests/ImageDownloaderTests/testSessionDataTaskConcurrentAccessIsThreadSafe \
  -only-testing:KingfisherTests/ImageDownloaderTests/testForceCancelConcurrentWithAddCallbackIsThreadSafe

Full macOS suite passes (666 tests).

Ref: #2539

started was written by resume() without synchronization while
ImageDownloader.startDownloadTask reads it from other threads — the
same class of race as the forceCancel one fixed in #2539. Guard it
with the existing lock, making resume() an atomic check-and-set with
task.resume() called outside the lock.

Also condense the forceCancel comment introduced in #2539 and add a
single combined stress test hammering all mutating and reading
surfaces of SessionDataTask concurrently (addCallback, forceCancel,
resume, started, containsCallbacks, didReceiveData, mutableDataCount,
completeAndRemoveAllCallbacks). Like the #2539 regression test, it is
meaningful mainly under Thread Sanitizer:

  xcodebuild test -project Kingfisher.xcodeproj -scheme Kingfisher \
    -destination 'platform=macOS' -enableThreadSanitizer YES \
    -only-testing:KingfisherTests/ImageDownloaderTests/testSessionDataTaskConcurrentAccessIsThreadSafe
@onevcat onevcat force-pushed the fix-force-cancel-race branch from 8a84c2b to 4baa44e Compare July 5, 2026 16:26
@onevcat onevcat changed the title Fix data races on SessionDataTask forceCancel and started Fix data race on SessionDataTask.started and tighten concurrency tests Jul 5, 2026
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.

1 participant