Thread: Implement more functionality #1162
Conversation
- Eliminate _compiler_crash_fix() as no crashes are observed anymore. - Re-enable TestThread.swift test cases.
|
@swift-ci please test |
|
Looks like ssh to github.com failed |
|
Yeah, timeout. I'll give it another try. |
|
@swift-ci please test |
|
|
||
| Boolean _CFIsMainThread(void) { | ||
| return pthread_main_np() == 1; | ||
| } |
There was a problem hiding this comment.
This is meant to be a Darwin implementation then?
There was a problem hiding this comment.
I'm wondering if we should make that #else more platform specific. Other than that, this looks great to me.
There was a problem hiding this comment.
I wasnt sure what other platforms support it, I thought FreeBSD might have it as well. Thought it would be easier to leave it open and then a version of _CFIsMainThread can be added to platforms as they need it.
There was a problem hiding this comment.
It also works for Windows since there is a #define pthread_main_np _NS_pthread_main_np wrapped inside a #if DEPLOYMENT_TARGET_WINDOWS
There was a problem hiding this comment.
Ok, I guess we'll hear back from people if this breaks their compile.
|
Can you update the |
|
Done, although I think some more test coverage will be needed at some point. |
TestFoundation/TestThread.swift
Outdated
| thread.start() | ||
|
|
||
| condition.lock() | ||
| if !started { |
There was a problem hiding this comment.
isn't this a race on started? what if the thread takes a while to start up and the condition lock here is locked, then started will be false and the condition wont wait.
There was a problem hiding this comment.
also this could potentially be waiting forever (if there was a problem with Thread), perhaps a semaphore with a timeout might be a good idea?
There was a problem hiding this comment.
I actually just copied the test_threadStart() function. You are correct that with a delay starting up the thread, !started might be false and the .wait() will never happen.
There is a pthread_cond_timedwait() function to timeout a .wait() so I will see if I can find a way to incorporate that to handle the thread never calling .broadcast().
I notice that NSLock also has 2 time bounded lock functions that need implementing so I might see if I can implement and incorporate them.
There was a problem hiding this comment.
Looks like NSCondition wait(until limit: Date) (using pthread_cond_timedwait()) has already been implemented so I will update to use that.
There was a problem hiding this comment.
I removed the started flag and used wait(until:) (including fixing a bug with the implementation) and then tried some tests including not starting the thread or making the thread take too long, and the timeout worked fine under those conditions.
I gave it 10seconds timeout which should cover all cases except incredibly high server load and if thats the case -CI will have other problems anyway
- Fix NSCondition.wait(until:) to correctly calculate the timespec to use for the timeout. - Fix test_threadStart() and test_mainThread() to eliminate the `started' flag which could have a race condition and instead use NSCondition.wait(until:) to timeout the test thread, in case it does not start up or call the broadcast() function.
|
@phausler Does this look ok now? |
|
@swift-ci please test |
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
Eliminate _compiler_crash_fix() as no crashes are observed anymore.
Re-enable TestThread.swift test cases.
I exposed
_CFMainPThreadsincepthread_main_thread_np()is a#defineand not a real function on linux and so doesn't export to swift correctly.I used
_CFIsMainThreadinstead ofpthread_main_npfor consistency and to avoid the local definition ofpthread_main_np()inOperation.swiftnot having the same semantics aspthread_main_np()on macOS where it can return -1.