Skip to content

Full Concurrency-safety#6

Merged
henrik-dmg merged 9 commits into
mainfrom
feature/concurrency
Mar 20, 2026
Merged

Full Concurrency-safety#6
henrik-dmg merged 9 commits into
mainfrom
feature/concurrency

Conversation

@henrik-dmg

@henrik-dmg henrik-dmg commented Jul 21, 2025

Copy link
Copy Markdown
Owner

This PR is bringing full concurrency-safety, especially to HPNetworkMock. It breaks the API though, so it'll be a new major version

Signed-off-by: Henrik Panhans <henrik@panhans.dev>
Signed-off-by: Henrik Panhans <henrik@panhans.dev>
@henrik-dmg henrik-dmg requested a review from Copilot July 21, 2025 20:03

Copilot AI 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.

Pull Request Overview

This PR implements full concurrency safety for the HPNetwork library, particularly focusing on the HPNetworkMock component. The changes modernize the codebase to use Swift 6's concurrency features and ensure thread safety across all networking operations.

  • Refactors NetworkClientMock to use an internal actor for thread-safe state management
  • Updates test files to use async/await patterns and removes callback-based approaches
  • Adds Sendable conformance to protocols and types throughout the networking stack

Reviewed Changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
Tests/HPNetworkTests/NetworkClientMockTests.swift Converts from lazy property to factory method, updates all tests to use async/await
Tests/HPNetworkTests/DownloadRequestTests.swift Simplifies test to use async/await and updates mock registration
Tests/HPNetworkTests/DataRequestTests.swift Similar async/await conversion and mock registration update
Sources/HPNetworkMock/URLSessionMock.swift Restructures with MockedRequestStore actor and adds availability constraints
Sources/HPNetworkMock/NetworkClientMock.swift Major refactor using internal actor for state management and async APIs
Sources/HPNetwork/Responses/NetworkResponse.swift Removes initializer and adds Sendable conformance
Sources/HPNetwork/Requests/NetworkRequest.swift Updates protocol with Sendable conformance and simplifies API
Sources/HPNetwork/Requests/DownloadRequest.swift Updates to use new async task-based API
Sources/HPNetwork/Requests/DataRequest.swift Similar task-based API updates
Sources/HPNetwork/NetworkClient.swift Simplifies protocol and implementation for new async patterns
Sources/HPNetwork/ConnectionMonitor.swift Adds @unchecked Sendable and main actor state updates
Sources/HPNetwork/Authorization/Authorization.swift Adds Sendable conformance
Package.swift Updates Swift tools version to 6.1

@@ -21,19 +21,6 @@ public struct NetworkResponse<Output> {
/// The time that elapsed during the processing of the network request's result.
public let processingDuration: TimeInterval

Copilot AI Jul 21, 2025

Copy link

Choose a reason for hiding this comment

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

The public initializer for NetworkResponse has been removed, but this breaks the ability to create NetworkResponse instances. The initializer should be restored or an alternative creation method should be provided.

Suggested change
/// Public initializer for creating a `NetworkResponse` instance.
public init(
output: Output,
url: URL,
response: HTTPResponse,
networkingDuration: TimeInterval,
processingDuration: TimeInterval
) {
self.output = output
self.url = url
self.response = response
self.networkingDuration = networkingDuration
self.processingDuration = processingDuration
}

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

No, NetworkResponse should not be constructible manually by the API consumer

import Network

public final class ConnectionMonitor: ObservableObject {
public final class ConnectionMonitor: ObservableObject, @unchecked Sendable {

Copilot AI Jul 21, 2025

Copy link

Choose a reason for hiding this comment

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

Using @unchecked Sendable requires careful justification. The class has mutable state (currentPath) that's accessed from multiple threads. Consider using proper synchronization or making this a proper Sendable type with actor isolation.

Suggested change
public final class ConnectionMonitor: ObservableObject, @unchecked Sendable {
public actor ConnectionMonitor: ObservableObject {

Copilot uses AI. Check for mistakes.
Comment on lines +82 to +85
do {
_ = try await networkClient.schedule(request).value
XCTFail("Request should not succeed")
} catch {

Copilot AI Jul 21, 2025

Copy link

Choose a reason for hiding this comment

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

This XCTFail will never be reached because the previous line try await networkClient.schedule(request).value will throw an exception if the request fails. The success case should be handled differently or the test logic should be restructured.

Copilot uses AI. Check for mistakes.
Signed-off-by: Henrik Panhans <henrik@panhans.dev>
Signed-off-by: Henrik Panhans <henrik@panhans.dev>
Signed-off-by: Henrik Panhans <henrik@panhans.dev>
@codecov

codecov Bot commented Jul 21, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.91919% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.79%. Comparing base (1bfc33b) to head (742d0a5).
⚠️ Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
Sources/HPNetwork/Requests/NetworkRequest.swift 0.00% 4 Missing ⚠️
Sources/HPNetworkMock/URLSessionMock.swift 50.00% 2 Missing ⚠️
Sources/HPNetworkMock/NetworkClientMock.swift 95.45% 1 Missing ⚠️
Sources/HPNetworkMock/URLRequestMockStore.swift 93.33% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main       #6       +/-   ##
===========================================
+ Coverage   80.28%   90.79%   +10.51%     
===========================================
  Files          13       14        +1     
  Lines         284      315       +31     
===========================================
+ Hits          228      286       +58     
+ Misses         56       29       -27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: Henrik Panhans <henrik@panhans.dev>
Signed-off-by: Henrik Panhans <henrik@panhans.dev>
Signed-off-by: Henrik Panhans <henrik@panhans.dev>
Signed-off-by: Henrik Panhans <henrik@panhans.dev>
@henrik-dmg henrik-dmg merged commit 742d0a5 into main Mar 20, 2026
4 checks passed
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