Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove setter from Atomic to prevent API misuse, fix TSAN error in network interceptor #1538

Merged
merged 4 commits into from
Nov 25, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions Sources/Apollo/NetworkFetchInterceptor.swift
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import Foundation
#if !COCOAPODS
import ApolloCore
#endif

/// An interceptor which actually fetches data from the network.
public class NetworkFetchInterceptor: ApolloInterceptor, Cancellable {
let client: URLSessionClient
private var currentTask: URLSessionTask?
private var currentTask: Atomic<URLSessionTask?> = Atomic(nil)

/// Designated initializer.
///
Expand All @@ -29,13 +32,13 @@ public class NetworkFetchInterceptor: ApolloInterceptor, Cancellable {
return
}

self.currentTask = self.client.sendRequest(urlRequest) { [weak self] result in
let task = self.client.sendRequest(urlRequest) { [weak self] result in
guard let self = self else {
return
}

defer {
self.currentTask = nil
self.currentTask.mutate { $0 = nil }
}

guard chain.isNotCancelled else {
Expand All @@ -57,9 +60,15 @@ public class NetworkFetchInterceptor: ApolloInterceptor, Cancellable {
completion: completion)
}
}

self.currentTask.mutate { $0 = task }
}

public func cancel() {
self.currentTask?.cancel()
guard let task = self.currentTask.value else {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this could just be self.currentTask.value?.cancel(), but I assume you prefer this because you find it more readable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

somewhat, yep.

return
}

task.cancel()
}
}
2 changes: 1 addition & 1 deletion Sources/Apollo/RequestChain.swift
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ public class RequestChain: Cancellable {

/// Cancels the entire chain of interceptors.
public func cancel() {
self.isCancelled.value = true
self.isCancelled.mutate { $0 = true }

// If an interceptor adheres to `Cancellable`, it should have its in-flight work cancelled as well.
for interceptor in self.interceptors {
Expand Down
2 changes: 1 addition & 1 deletion Sources/Apollo/URLSessionClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ open class URLSessionClient: NSObject, URLSessionDelegate, URLSessionTaskDelegat
///
/// NOTE: This must be called from the `deinit` of anything holding onto this client in order to break a retain cycle with the delegate.
public func invalidate() {
self.hasBeenInvalidated.value = true
self.hasBeenInvalidated.mutate { $0 = true }
func cleanup() {
self.session = nil
self.clearAllTasks()
Expand Down
20 changes: 6 additions & 14 deletions Sources/ApolloCore/Atomic.swift
Original file line number Diff line number Diff line change
Expand Up @@ -12,23 +12,15 @@ public class Atomic<T> {
_value = value
}

/// The current value.
/// The current value. Read-only. To update the underlying value, use `mutate`.
public var value: T {
get {
lock.lock()
defer { lock.unlock() }

return _value
}
set {
lock.lock()
defer { lock.unlock() }

_value = newValue
}
lock.lock()
defer { lock.unlock() }

return _value
}

/// Mutates the underlying value within a lock. Mostly useful for mutating the contents of `Atomic` wrappers around collections.
/// Mutates the underlying value within a lock.
/// - Parameter block: The block to execute to mutate the value.
public func mutate(block: (inout T) -> Void) {
lock.lock()
Expand Down
2 changes: 1 addition & 1 deletion Sources/ApolloTestSupport/MockURLSession.swift
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public final class MockURLSessionClient: URLSessionClient {
public override func sendRequest(_ request: URLRequest,
rawTaskCompletionHandler: URLSessionClient.RawCompletion? = nil,
completion: @escaping URLSessionClient.Completion) -> URLSessionTask {
self.lastRequest.value = request
self.lastRequest.mutate { $0 = request }

// Capture data, response, and error instead of self to ensure we complete with the current state
// even if it is changed before the block runs.
Expand Down
18 changes: 10 additions & 8 deletions Sources/ApolloWebSocket/WebSocketTransport.swift
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ public class WebSocketTransport {
}

public func closeConnection() {
self.reconnect.value = false
self.reconnect.mutate { $0 = false }

let str = OperationMessage(type: .connectionTerminate).rawMessage
processingQueue.async {
Expand Down Expand Up @@ -325,28 +325,28 @@ public class WebSocketTransport {

private func reconnectWebSocket() {
let oldReconnectValue = reconnect.value
self.reconnect.value = false
self.reconnect.mutate { $0 = false }

self.websocket.disconnect()
self.websocket.connect()

reconnect.value = oldReconnectValue
self.reconnect.mutate { $0 = oldReconnectValue }
}

/// Disconnects the websocket while setting the auto-reconnect value to false,
/// allowing purposeful disconnects that do not dump existing subscriptions.
/// NOTE: You will receive an error on the subscription (should be a `Starscream.WSError` with code 1000) when the socket disconnects.
/// ALSO NOTE: To reconnect after calling this, you will need to call `resumeWebSocketConnection`.
public func pauseWebSocketConnection() {
self.reconnect.value = false
self.reconnect.mutate { $0 = false }
self.websocket.disconnect()
}

/// Reconnects a paused web socket.
///
/// - Parameter autoReconnect: `true` if you want the websocket to automatically reconnect if the connection drops. Defaults to true.
public func resumeWebSocketConnection(autoReconnect: Bool = true) {
self.reconnect.value = autoReconnect
self.reconnect.mutate { $0 = autoReconnect }
self.websocket.connect()
}
}
Expand Down Expand Up @@ -394,7 +394,7 @@ extension WebSocketTransport: NetworkTransport {
extension WebSocketTransport: WebSocketDelegate {

public func websocketDidConnect(socket: WebSocketClient) {
self.error.value = nil
self.error.mutate { $0 = nil }
initServer()
if reconnected {
self.delegate?.webSocketTransportDidReconnect(self)
Expand All @@ -419,10 +419,12 @@ extension WebSocketTransport: WebSocketDelegate {
public func websocketDidDisconnect(socket: WebSocketClient, error: Error?) {
// report any error to all subscribers
if let error = error {
self.error.value = WebSocketError(payload: nil, error: error, kind: .networkError)
self.error.mutate { $0 = WebSocketError(payload: nil,
error: error,
kind: .networkError) }
self.notifyErrorAllHandlers(error)
} else {
self.error.value = nil
self.error.mutate { $0 = nil }
}

self.delegate?.webSocketTransport(self, didDisconnectWithError: self.error.value)
Expand Down