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

Sr 1872 using runloopsource in wait for expectation #314

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pranavshenoy
Copy link

SR-1872 Replacing polling expectations to check whether an expectation is fulfilled with RunloopSource

@pranavshenoy
Copy link
Author

@compnerd can we test the changes??

@compnerd
Copy link
Member

@swift-ci please test

@pranavshenoy
Copy link
Author

Previous discussions: #304

@stmontgomery stmontgomery self-requested a review November 24, 2020 15:40
Copy link
Contributor

@stmontgomery stmontgomery left a comment

Choose a reason for hiding this comment

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

Hi @pranavshenoy, thanks for taking this on. I have familiarized myself with the discussion on the previous PR, #304, and I have a few questions. One "larger" question, and some smaller inline suggestions about the code…

My biggest question about this change is: Is it missing the step of "signaling" the run loop source? I had to refresh my memory on how these work, but IIRC the idea behind run loop sources is that, once you assign a source to a run loop and begin running the run loop, it begins and doesn't finish until either its source is signaled or it times out.

In this PR, it looks like you're adding the source to the run loop, and at the very end (once it's no longer needed) you're calling invalidate() on the source… but I don't see a place where RunLoop._Source.signal() is called. My understanding was that, each time one of the waiter's XCTestExpectations is fulfilled, we would need to signal() the source to cause the run loop to spin again. And looking at Apple's documentation for CFRunLoopSourceSignal (the underlying C API), it seems like one would need to call CFRunLoopWakeUp after signal() as well, since we'd want the run loop to process it and fire immediately. (I'm not sure there's a Swift RunLoop._Source wrapper for CFRunLoopWakeUp, yet.)

Can you clarify if I am misunderstanding something or if this signaling happens a different way?

@@ -405,7 +389,7 @@ extension XCTWaiter: ManageableWaiter {
dispatchPrecondition(condition: .onQueue(XCTWaiter.subsystemQueue))

queue_validateExpectationFulfillment(dueToTimeout: true)
manager!.queue_handleWatchdogTimeout(of: self)
manager?.queue_handleWatchdogTimeout(of: self)
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the reason for changing this, did you encounter a case where the force-unwrap failed after your changes? I vaguely recall using force unwrap intentionally here to represent the expectation that, at this point in the waiter lifecycle, it definitely should still have a WaiterManager, but I may have missed something or this PR could have changed that invariant.

Copy link
Author

Choose a reason for hiding this comment

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

Initially, this was causing crashes. Hence, felt optional unwrapping would be safer to avoid crashes.
Have reverted the changes. Thanks.

Copy link
Author

Choose a reason for hiding this comment

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

@stmontgomery was looking into the root cause of the crash. It looks like for the scenario where the duration of wait() is less than or equal to zero (there are two functional test cases for this), there exists a race-condition between queue_handleWatchdogTimeout() and self.manager = nil with the latest changes. This is because the isFinished property, which has subsystemQueue.sync that prevents the race-condition, is not needed in the latest changes.

Please let me know if the approach needs to be changed.

@@ -117,7 +117,7 @@ open class XCTWaiter {
internal var waitSourceLocation: SourceLocation?
private weak var manager: WaiterManager<XCTWaiter>?
private var runLoop: RunLoop?

private var runLoopSource: RunLoop._Source?
Copy link
Contributor

Choose a reason for hiding this comment

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

I notice the RunLoop._Source type and several of its related RunLoop methods are marked as deprecated in Foundation… does that mean this PR will introduce new build warnings to the XCTest codebase? I wonder what we can do about that, since Swift has no way to "silence" those warnings.

@pranavshenoy
Copy link
Author

pranavshenoy commented Dec 13, 2020

Hi @stmontgomery
Thanks for the detailed review.

From my understanding of the code, the RunLoop in the primitiveWait() is expecting only invalidation either when all expectations are fulfilled or the time is elapsed. I believe invalidation of runLoopSource would suffice for this requirement and “signaling” the runLoopSource might not be required. When fulfill() is invoked for an expectation, the validateExpectations() method within it checks whether all of the waiter’s XCTestExpectations are fulfilled. If all are fulfilled, the queue_finished() is invoked, which in turn invokes cancelPrimitiveWait(), invalidating the RunLoop and resuming the execution of wait().

From my understanding of the task, the while loop in the wait(), which checks for !isFinished. must be replaced with RunLoopSource. Additionally, I feel the same could be achieved without RunLoopSource (starting the RunLoop and stopping it).

Please let me know if I am misinterpreting the code and/or the task.

@pranavshenoy pranavshenoy force-pushed the SR-1872-using-runloopsource-in-waitForExpectation branch 2 times, most recently from 4b7289d to 9b64c4a Compare December 15, 2020 18:50
@pranavshenoy pranavshenoy force-pushed the SR-1872-using-runloopsource-in-waitForExpectation branch from 9b64c4a to 34c57d1 Compare December 15, 2020 18:59
@pranavshenoy
Copy link
Author

@compnerd could you please trigger the build? Thanks

@spevans
Copy link
Contributor

spevans commented Dec 17, 2020

@swift-ci test

@pranavshenoy
Copy link
Author

pranavshenoy commented Dec 29, 2020

@stmontgomery
from this FIXME, I feel that the expectation of the task was to signal RunLoopSource when each expectation is fulfilled() instead of polling. But with the additional capabilities like InverseExpectation, innerWaiter and outerWaiter, this is handled by validateExpectations(). Hence I feel, calling runLoop.run(until: Date(timeIntervalSinceNow: timeout)) in wait() and letting queue_finish() cancel the RunLoop would suffice.

I tried it out locally and all tests were successful.

Could you please tell me if there is some gap with this approach that I am missing? If this one is good, I can proceed and make the necessary changes.

Thanks

@pranavshenoy
Copy link
Author

@stmontgomery @millenomi Can you share your thoughts on this?

@pranavshenoy
Copy link
Author

@stmontgomery @millenomi @compnerd Could you please provide your inputs on this?

Thanks

@pranavshenoy
Copy link
Author

@stmontgomery @millenomi @compnerd Could you please provide your inputs on this?

@stmontgomery @millenomi @compnerd Could you please provide your inputs on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants