Skip to content

[Operation] add willChangeValue() and didChangeValue() #836

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 23, 2017
Merged

[Operation] add willChangeValue() and didChangeValue() #836

merged 3 commits into from
Mar 23, 2017

Conversation

ianpartridge
Copy link
Contributor

Following on from the discussion on the swift-corelibs-dev mailing list, here's a PR to add willChangeValue() and didChangeValue() to Operation, in support of operations that are asynchronous from an operation queue.

@ianpartridge ianpartridge changed the title Operation: add willChangeValue() and didChangeValue() [Operation] add willChangeValue() and didChangeValue() Jan 24, 2017
phausler
phausler reviewed Jan 25, 2017
View reviewed changes
Foundation/NSOperation.swift
@@ -47,7 +47,6 @@ open class Operation : NSObject {
#endif
}

/// - Note: Operations that are asynchronous from the execution of the operation queue itself are not supported since there is no KVO to trigger the finish.
open func start() {
Copy link
Contributor

Choose a reason for hiding this comment

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

So the problem now is that if someone overrides main but not start and detaches from the current operation's queue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we document and live with that for now to unblock this particular use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any updates on this? The limitation @phausler identifies seems less serious than the current one.

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 like the lesser of two evils. so seems like a decent work-around until we have a better answer

Copy link
Contributor

Choose a reason for hiding this comment

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

could you perhaps add a simple unit test to verify the expected behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@phausler @parkera I've added a test for this. Please could you review and run CI?

@parkera
Copy link
Contributor

parkera commented Feb 17, 2017

@swift-ci please test

@alblue
Copy link
Contributor

alblue commented Feb 22, 2017

There is a test failure on this - can you investigate and fix if necessary?

Test Case 'TestNSOperationQueue.test_AsyncOperation' started at 15:38:06.845
TestFoundation/TestNSOperationQueue.swift:80: error: TestNSOperationQueue.test_AsyncOperation : XCTAssertTrue failed - 
TestFoundation/TestNSOperationQueue.swift:81: error: TestNSOperationQueue.test_AsyncOperation : XCTAssertFalse failed -

@ianpartridge
Copy link
Contributor Author

Will do. Race condition, I think.

phausler
phausler reviewed Feb 22, 2017
View reviewed changes
TestFoundation/TestNSOperationQueue.swift Outdated

private let queue = DispatchQueue(label: "async.operation.queue")

private var _executing = false
Copy link
Contributor

Choose a reason for hiding this comment

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

these two variables must be changed under the same lock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean where they are updated at the end of the queue.async() block below?

What's the best way to do this? NSLock?

Copy link
Contributor

Choose a reason for hiding this comment

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

NSLock should work, yes.

@ianpartridge
Copy link
Contributor Author

@parkera Updated based on feedback - thanks. Please test and merge if you are happy.

@parkera
Copy link
Contributor

parkera commented Mar 21, 2017

@swift-ci please test

@ianpartridge
Copy link
Contributor Author

@parkera tests passed 😄

@parkera
Copy link
Contributor

parkera commented Mar 23, 2017

Thanks!

@parkera
Copy link
Contributor

parkera commented Mar 23, 2017

@swift-ci test and merge

@swift-ci swift-ci merged commit bd53bb6 into swiftlang:master Mar 23, 2017
@ianpartridge ianpartridge deleted the async-operation branch March 23, 2017 16:55
@rhyshort rhyshort mentioned this pull request Mar 23, 2017
Closed
@ianpartridge ianpartridge mentioned this pull request Apr 12, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parkera parkera parkera left review comments

@phausler phausler phausler left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ianpartridge @parkera @alblue @phausler @swift-ci