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-5871: TestThread.test_threadStart failing sporadically on Ubuntu 14.04 #1218

Merged
merged 1 commit into from Sep 20, 2017

Conversation

spevans
Copy link
Collaborator

@spevans spevans commented Sep 14, 2017

  • Lock the NSCondition variable before creating the thread,
    otherwise the thread could broadcast on the condition variable
    before the parent has waited on it.

…4.04

- Lock the NSCondition variable before creating the thread,
  otherwise the thread could broadcast on the condition variable
  before the parent has waited on it.
@spevans
Copy link
Collaborator Author

spevans commented Sep 14, 2017

@swift-ci please test

@spevans
Copy link
Collaborator Author

spevans commented Sep 18, 2017

@ianpartridge I reread the man page for condition variables so I think these are being used in the correct order now. Worth a merge to see if it fixes the issue?

@@ -41,14 +41,15 @@ class TestThread : XCTestCase {

func test_threadStart() {
let condition = NSCondition()
condition.lock()
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this end up locking on the condition twice? Is that supported?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The first lock happens in the parent thread - it acquires the lock.
The second lock happens in the newly created lock - it blocks waiting to acquire it.
When the parent thread calls condition.wait() the lock is unlocked and then the 2nd thread acquires it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yes - the change makes sense in that regard.

@ianpartridge
Copy link
Collaborator

@swift-ci please test and merge

1 similar comment
@spevans
Copy link
Collaborator Author

spevans commented Sep 20, 2017

@swift-ci please test and merge

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.

None yet

4 participants