{"action":"created","issue":{"url":"https://api.github.com/repos/danthorpe/Operations/issues/352","repository_url":"https://api.github.com/repos/danthorpe/Operations","labels_url":"https://api.github.com/repos/danthorpe/Operations/issues/352/labels{/name}","comments_url":"https://api.github.com/repos/danthorpe/Operations/issues/352/comments","events_url":"https://api.github.com/repos/danthorpe/Operations/issues/352/events","html_url":"https://github.com/danthorpe/Operations/issues/352","id":162982885,"number":352,"title":"GroupOperation.cancel() race condition(s)? (assertion failed: Cannot add new operations to a group after the group has completed.)","user":{"login":"swiftlyfalling","id":18540905,"avatar_url":"https://avatars.githubusercontent.com/u/18540905?v=3","gravatar_id":"","url":"https://api.github.com/users/swiftlyfalling","html_url":"https://github.com/swiftlyfalling","followers_url":"https://api.github.com/users/swiftlyfalling/followers","following_url":"https://api.github.com/users/swiftlyfalling/following{/other_user}","gists_url":"https://api.github.com/users/swiftlyfalling/gists{/gist_id}","starred_url":"https://api.github.com/users/swiftlyfalling/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/swiftlyfalling/subscriptions","organizations_url":"https://api.github.com/users/swiftlyfalling/orgs","repos_url":"https://api.github.com/users/swiftlyfalling/repos","events_url":"https://api.github.com/users/swiftlyfalling/events{/privacy}","received_events_url":"https://api.github.com/users/swiftlyfalling/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/danthorpe/Operations/labels/bug","name":"bug","color":"fc2929"}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":9,"created_at":"2016-06-29T17:31:53Z","updated_at":"2016-07-02T12:02:13Z","closed_at":null,"body":"The code below randomly results in:\r\n`assertion failed: Cannot add new operations to a group after the group has completed.: file /Operations/Sources/Core/Shared/GroupOperation.swift, line 197`\r\n\r\n(Testing using development branch.)\r\n\r\nRepro code:\r\n```swift\r\nfinal class TestGroupOperation: GroupOperation {\r\n    \r\n    private var currentOperation: TestOperation!\r\n    \r\n    init(number: Int) {\r\n        super.init(operations:[])\r\n        self.name = \"GroupOp_\\(number)\"\r\n        let firstOperation = generateOperation()\r\n        self.currentOperation = firstOperation\r\n        self.addOperation(firstOperation)\r\n    }\r\n    \r\n    func generateOperation() -> TestOperation {\r\n        let newOperation = TestOperation()\r\n        newOperation.addObserver(WillFinishObserver { [weak self] op, errors in\r\n            print(\"\\((self?.operationName ?? \"\")):WillFinishObserver\")\r\n            guard let operation = op as? TestOperation, strongSelf = self else { return }\r\n            \r\n            guard !strongSelf.cancelled else {\r\n                print(\"\\((self?.operationName ?? \"\")): Stopping because cancelled\")\r\n                return\r\n            }\r\n            \r\n            guard !strongSelf.finished else {\r\n                print(\"\\((self?.operationName ?? \"\")): Stopping because finished\")\r\n                return\r\n            }\r\n            \r\n            let nextOperation = strongSelf.generateOperation()\r\n            print(\"\\((self?.operationName ?? \"\")):AddOperation\")\r\n            strongSelf.addOperation(nextOperation) // <--- This occasionally results in an assertion failure, despite checking for .cancelled AND .finished above?\r\n        })\r\n        return newOperation\r\n    }\r\n}\r\n\r\nfinal class TestOperation: Operation {\r\n    override func execute() {\r\n        guard !cancelled else { return }\r\n        // simulate long-running task by not finishing\r\n    }\r\n}\r\n\r\n// Randomly results in assertion failure when run in iOS 9.3 Simulator or Mac OS X 10.11:\r\n// Occasionally results in other crashes - including within NSOperation:\r\nlet queue = OperationQueue()\r\nvar currentGroupOperation = TestGroupOperation(number: 0)\r\nqueue.addOperation(currentGroupOperation)\r\nfor i in 1..<5000 {\r\n     currentGroupOperation.cancel()\r\n     currentGroupOperation = TestGroupOperation(number: i)\r\n     queue.addOperation(currentGroupOperation) // <--- Less frequently, there are different crashes triggered by this line.\r\n}\r\n\r\n```\r\n\r\nSome other crashes I've seen are:\r\n\r\n```\r\nassertion failed: Attempting to perform illegal cyclic state transition, Finishing -> Finishing for operation: GroupOp_91 #UUID.: file /Operations/Sources/Core/Shared/Operation.swift, line 354\r\n```\r\n\r\nand:\r\n```\r\n* thread #9: tid = 0x24ecf, 0x00007fff957f9103 libobjc.A.dylib`objc_release + 35, queue = 'NSOperationQueue 0x608000043510 :: NSOperation 0x608000140d10 (QOS: USER_INTERACTIVE)', stop reason = EXC_BAD_ACCESS (code=1, address=0x40dedeadbec8)\r\n  * frame #0: 0x00007fff957f9103 libobjc.A.dylib`objc_release + 35\r\n    frame #1: 0x00007fff957f7b3b libobjc.A.dylib`(anonymous namespace)::AutoreleasePoolPage::pop(void*) + 477\r\n    frame #2: 0x0000000100910cc5 libdispatch.dylib`_dispatch_client_callout + 8\r\n    frame #3: 0x0000000100916112 libdispatch.dylib`_dispatch_queue_drain + 351\r\n    frame #4: 0x000000010091de24 libdispatch.dylib`_dispatch_queue_invoke + 557\r\n    frame #5: 0x0000000100914dab libdispatch.dylib`_dispatch_root_queue_drain + 1226\r\n    frame #6: 0x00000001009148a5 libdispatch.dylib`_dispatch_worker_thread3 + 106\r\n    frame #7: 0x0000000100973336 libsystem_pthread.dylib`_pthread_wqthread + 1129\r\n    frame #8: 0x0000000100970f91 libsystem_pthread.dylib`start_wqthread + 13\r\n```\r\n\r\nThoughts?"},"comment":{"url":"https://api.github.com/repos/danthorpe/Operations/issues/comments/230098494","html_url":"https://github.com/danthorpe/Operations/issues/352#issuecomment-230098494","issue_url":"https://api.github.com/repos/danthorpe/Operations/issues/352","id":230098494,"user":{"login":"danthorpe","id":309420,"avatar_url":"https://avatars.githubusercontent.com/u/309420?v=3","gravatar_id":"","url":"https://api.github.com/users/danthorpe","html_url":"https://github.com/danthorpe","followers_url":"https://api.github.com/users/danthorpe/followers","following_url":"https://api.github.com/users/danthorpe/following{/other_user}","gists_url":"https://api.github.com/users/danthorpe/gists{/gist_id}","starred_url":"https://api.github.com/users/danthorpe/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/danthorpe/subscriptions","organizations_url":"https://api.github.com/users/danthorpe/orgs","repos_url":"https://api.github.com/users/danthorpe/repos","events_url":"https://api.github.com/users/danthorpe/events{/privacy}","received_events_url":"https://api.github.com/users/danthorpe/received_events","type":"User","site_admin":false},"created_at":"2016-07-02T12:02:13Z","updated_at":"2016-07-02T12:02:13Z","body":"@swiftlyfalling - really good points here about `GroupOperation` and `Operation.state`.\r\n\r\nHmm, what to do...\r\n\r\n1. I think we need to flesh out the stress tests with test cases like your sample code here to trigger failures.\r\n2. Address thread safety issues of `Operation` state changes - especially cancelled. Which does not acquire the state lock.\r\n3. Address the cancellation behaviour of `GroupOperation` mentioned above - especially if it is cancelled before it is executed.\r\n\r\nIn terms of actually doing this work - at the moment I think this needs to be v3.1 or v3.2 type stuff. At the moment `development` is already quite far ahead of `master`, and I do not want to further hold it back. Once v3.0 is released, we can look at fixing these issues as point releases on v3 as we move towards v4 on the roadmap (as described in #343).\r\n\r\nWhat do you think?\r\n"}}
{"actor":{"display_login":"danthorpe"}}