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
[Operation] add willChangeValue() and didChangeValue() #836
Conversation
@@ -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() { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6e2cca3
to
cd89e45
Compare
@swift-ci please test |
There is a test failure on this - can you investigate and fix if necessary?
|
Will do. Race condition, I think. |
|
||
private let queue = DispatchQueue(label: "async.operation.queue") | ||
|
||
private var _executing = false |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NSLock
should work, yes.
15dcc62
to
ed6bf4d
Compare
ed6bf4d
to
6174b05
Compare
@parkera Updated based on feedback - thanks. Please test and merge if you are happy. |
@swift-ci please test |
@parkera tests passed 😄 |
Thanks! |
@swift-ci test and merge |
Following on from the discussion on the swift-corelibs-dev mailing list, here's a PR to add
willChangeValue()
anddidChangeValue()
toOperation
, in support of operations that are asynchronous from an operation queue.