From d03f5a6616bb44fb7916c98627e5302998a02d64 Mon Sep 17 00:00:00 2001 From: Harsh <6162866+harsh62@users.noreply.github.com> Date: Tue, 30 Apr 2024 13:37:48 -0400 Subject: [PATCH 1/2] fix(Auth): Refactoring state machine logic to fix memory leak (#3613) * fix(Auth): Fixing memory leaks happening because of the state machine * Update AmplifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/StateMachine/hierarchical-state-machine-swift/StateMachine.swift Co-authored-by: Di Wu * worked on review comment --------- Co-authored-by: Di Wu --- .../CancellableAsyncStream.swift | 43 +++++++++++++ .../StateAsyncSequence.swift | 36 ----------- .../StateMachine.swift | 63 +++++-------------- 3 files changed, 58 insertions(+), 84 deletions(-) create mode 100644 AmplifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/StateMachine/hierarchical-state-machine-swift/CancellableAsyncStream.swift delete mode 100644 AmplifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/StateMachine/hierarchical-state-machine-swift/StateAsyncSequence.swift diff --git a/AmplifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/StateMachine/hierarchical-state-machine-swift/CancellableAsyncStream.swift b/AmplifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/StateMachine/hierarchical-state-machine-swift/CancellableAsyncStream.swift new file mode 100644 index 0000000000..35ce4b65c1 --- /dev/null +++ b/AmplifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/StateMachine/hierarchical-state-machine-swift/CancellableAsyncStream.swift @@ -0,0 +1,43 @@ +// +// Copyright Amazon.com Inc. or its affiliates. +// All Rights Reserved. +// +// SPDX-License-Identifier: Apache-2.0 +// + +import Combine + +class CancellableAsyncStream: AsyncSequence { + + typealias AsyncIterator = AsyncStream.AsyncIterator + private let asyncStream: AsyncStream + private let cancellable: AnyCancellable? + + deinit { + cancel() + } + + init(asyncStream: AsyncStream, cancellable: AnyCancellable?) { + self.asyncStream = asyncStream + self.cancellable = cancellable + } + + convenience init(with publisher: AnyPublisher) { + var cancellable: AnyCancellable? + self.init(asyncStream: AsyncStream { continuation in + cancellable = publisher.sink { _ in + continuation.finish() + } receiveValue: { + continuation.yield($0) + } + }, cancellable: cancellable) + } + + func makeAsyncIterator() -> AsyncStream.AsyncIterator { + asyncStream.makeAsyncIterator() + } + + func cancel() { + cancellable?.cancel() + } +} diff --git a/AmplifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/StateMachine/hierarchical-state-machine-swift/StateAsyncSequence.swift b/AmplifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/StateMachine/hierarchical-state-machine-swift/StateAsyncSequence.swift deleted file mode 100644 index 04cf788b1f..0000000000 --- a/AmplifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/StateMachine/hierarchical-state-machine-swift/StateAsyncSequence.swift +++ /dev/null @@ -1,36 +0,0 @@ -// -// Copyright Amazon.com Inc. or its affiliates. -// All Rights Reserved. -// -// SPDX-License-Identifier: Apache-2.0 -// - -import Foundation - -class StateAsyncSequence: AsyncSequence { - - typealias Iterator = AsyncStream.Iterator - private var continuation: AsyncStream.Continuation! = nil - - private var asyncStream: AsyncStream! = nil - - init(bufferingPolicy: AsyncStream.Continuation.BufferingPolicy = .unbounded) { - asyncStream = AsyncStream( - Element.self, - bufferingPolicy: bufferingPolicy) { continuation in - self.continuation = continuation - } - } - - func makeAsyncIterator() -> Iterator { - asyncStream.makeAsyncIterator() - } - - func send(_ element: Element) { - continuation.yield(element) - } - - func cancel() { - continuation.finish() - } -} diff --git a/AmplifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/StateMachine/hierarchical-state-machine-swift/StateMachine.swift b/AmplifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/StateMachine/hierarchical-state-machine-swift/StateMachine.swift index 7b69423411..6fd47e52ad 100644 --- a/AmplifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/StateMachine/hierarchical-state-machine-swift/StateMachine.swift +++ b/AmplifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/StateMachine/hierarchical-state-machine-swift/StateMachine.swift @@ -5,15 +5,7 @@ // SPDX-License-Identifier: Apache-2.0 // -import Foundation - -/// Captures a weak reference to the value -class WeakWrapper where T: AnyObject { - private(set) weak var value: T? - init(_ value: T) { - self.value = value - } -} +import Combine /// Models, evolves, and processes effects for a system. /// @@ -31,15 +23,18 @@ actor StateMachine< EnvironmentType: Environment > where StateType: State { - /// AsyncSequences are invoked a minimum of one time: Each sequence receives the current - /// state as soon as `listen()` is invoked, and will receive each subsequent state change. - typealias StateChangeSequence = StateAsyncSequence - private let environment: EnvironmentType private let resolver: AnyResolver - private(set) var currentState: StateType - private var subscribers: [WeakWrapper>] + public var currentState: StateType { + currentStateSubject.value + } + + private let currentStateSubject: CurrentValueSubject + + deinit { + currentStateSubject.send(completion: .finished) + } init( resolver: ResolverType, @@ -48,22 +43,16 @@ actor StateMachine< ) where ResolverType: StateMachineResolver, ResolverType.StateType == StateType { self.resolver = resolver.eraseToAnyResolver() self.environment = environment - self.currentState = initialState ?? resolver.defaultState - - self.subscribers = [] + self.currentStateSubject = CurrentValueSubject(initialState ?? resolver.defaultState) } /// Start listening to state change updates. The current state and all subsequent state changes will be sent to the sequence. /// /// - Returns: An async sequence that get states asynchronously - func listen() -> StateChangeSequence { - let sequence = StateAsyncSequence() - let currentState = self.currentState - let wrappedSequence = WeakWrapper(sequence) - subscribers.append(wrappedSequence) - sequence.send(currentState) - return sequence + func listen() -> CancellableAsyncStream { + CancellableAsyncStream(with: currentStateSubject.eraseToAnyPublisher()) } + } extension StateMachine: EventDispatcher { @@ -88,33 +77,11 @@ extension StateMachine: EventDispatcher { ) if currentState != resolution.newState { - currentState = resolution.newState - subscribers.removeAll { item in - !notify(subscriberElement: item, about: resolution.newState) - } + currentStateSubject.send(resolution.newState) } execute(resolution.actions) } - /// - Parameters: - /// - subscriberElement: A weak wrapped async sequence - /// - newState: The new state to be sent - /// - Returns: true if the subscriber was notified, false if the wrapper reference was nil or a cancellation was pending - private func notify( - subscriberElement: WeakWrapper, - about newState: StateType - ) -> Bool { - - // If weak reference has become nil, do not process, and return false so caller can remove - // the subscription from the subscribers list - guard let sequence = subscriberElement.value else { - return false - } - - sequence.send(newState) - return true - } - private func execute(_ actions: [Action]) { guard !actions.isEmpty else { return From 6e2a881a01e573f2f413859bcb9b697270604bf2 Mon Sep 17 00:00:00 2001 From: Sebastian Villena <97059974+ruisebas@users.noreply.github.com> Date: Tue, 30 Apr 2024 13:39:32 -0400 Subject: [PATCH 2/2] chore: Control coverage through repository variable [skip ci] (#3656) --- .github/workflows/unit_test.yml | 59 +++++++++++++++++---------------- 1 file changed, 30 insertions(+), 29 deletions(-) diff --git a/.github/workflows/unit_test.yml b/.github/workflows/unit_test.yml index 1dbd2e4565..1e733c2060 100644 --- a/.github/workflows/unit_test.yml +++ b/.github/workflows/unit_test.yml @@ -41,7 +41,7 @@ concurrency: cancel-in-progress: ${{ github.ref_name != 'main'}} jobs: - unit-tests-without-coverage: + targets-without-coverage: name: ${{ matrix.scheme }} Unit Tests strategy: fail-fast: false @@ -54,7 +54,7 @@ jobs: scheme: ${{ matrix.scheme }} generate_coverage_report: false - unit-tests-with-coverage: + targets-with-coverage: name: ${{ matrix.scheme }} Unit Tests strategy: fail-fast: false @@ -76,40 +76,41 @@ jobs: uses: ./.github/workflows/run_unit_tests_platforms.yml with: scheme: ${{ matrix.scheme }} - generate_coverage_report: true + generate_coverage_report: ${{ vars.DISABLE_COVERAGE_REPORT != 'true' }} - # report-coverage: - # name: ${{ matrix.file.scheme }} Unit Tests - # needs: [unit-tests-with-coverage] - # strategy: - # fail-fast: false - # matrix: - # file: [ - # { scheme: Amplify, flags: 'Amplify,unit_tests' }, - # { scheme: AWSPluginsCore, flags: 'AWSPluginsCore,unit_tests' }, - # { scheme: AWSAPIPlugin, flags: 'API_plugin_unit_test,unit_tests' }, - # { scheme: AWSCloudWatchLoggingPlugin, flags: 'Logging_plugin_unit_test,unit_tests' }, - # { scheme: AWSCognitoAuthPlugin, flags: 'Auth_plugin_unit_test,unit_tests' }, - # { scheme: AWSDataStorePlugin, flags: 'DataStore_plugin_unit_test,unit_tests' }, - # { scheme: AWSLocationGeoPlugin, flags: 'Geo_plugin_unit_test,unit_tests' }, - # { scheme: AWSPredictionsPlugin, flags: 'Predictions_plugin_unit_test,unit_tests' }, - # { scheme: AWSPinpointAnalyticsPlugin, flags: 'Analytics_plugin_unit_test,unit_tests' }, - # { scheme: AWSPinpointPushNotificationsPlugin, flags: 'PushNotifications_plugin_unit_test,unit_tests' }, - # { scheme: AWSS3StoragePlugin, flags: 'Storage_plugin_unit_test,unit_tests' }, - # { scheme: CoreMLPredictionsPlugin, flags: 'CoreMLPredictions_plugin_unit_test,unit_tests' } - # ] - # uses: ./.github/workflows/upload_coverage_report.yml - # with: - # scheme: ${{ matrix.file.scheme }} - # flags: ${{ matrix.file.flags }} + report-coverage: + if: ${{ vars.DISABLE_COVERAGE_REPORT != 'true' }} + name: ${{ matrix.file.scheme }} Unit Tests + needs: [targets-with-coverage] + strategy: + fail-fast: false + matrix: + file: [ + { scheme: Amplify, flags: 'Amplify,unit_tests' }, + { scheme: AWSPluginsCore, flags: 'AWSPluginsCore,unit_tests' }, + { scheme: AWSAPIPlugin, flags: 'API_plugin_unit_test,unit_tests' }, + { scheme: AWSCloudWatchLoggingPlugin, flags: 'Logging_plugin_unit_test,unit_tests' }, + { scheme: AWSCognitoAuthPlugin, flags: 'Auth_plugin_unit_test,unit_tests' }, + { scheme: AWSDataStorePlugin, flags: 'DataStore_plugin_unit_test,unit_tests' }, + { scheme: AWSLocationGeoPlugin, flags: 'Geo_plugin_unit_test,unit_tests' }, + { scheme: AWSPredictionsPlugin, flags: 'Predictions_plugin_unit_test,unit_tests' }, + { scheme: AWSPinpointAnalyticsPlugin, flags: 'Analytics_plugin_unit_test,unit_tests' }, + { scheme: AWSPinpointPushNotificationsPlugin, flags: 'PushNotifications_plugin_unit_test,unit_tests' }, + { scheme: AWSS3StoragePlugin, flags: 'Storage_plugin_unit_test,unit_tests' }, + { scheme: CoreMLPredictionsPlugin, flags: 'CoreMLPredictions_plugin_unit_test,unit_tests' } + ] + uses: ./.github/workflows/upload_coverage_report.yml + with: + scheme: ${{ matrix.file.scheme }} + flags: ${{ matrix.file.flags }} unit-test-pass-confirmation: runs-on: ubuntu-latest name: Confirm Passing Unit Tests if: ${{ !cancelled() }} needs: [ - unit-tests-with-coverage, - unit-tests-without-coverage + targets-with-coverage, + targets-without-coverage ] env: EXIT_CODE: ${{ contains(needs.*.result, 'failure') && 1 || 0 }}