fix Issue 8960 - Unable to set thread priority

[MartinNowak]

• ignore FreeBSD error when get-/setting priority on terminated thread
• this also ignores invalid thread handles

[ghost]

Note, that situation on linux and windows has fundamentally the same problem. Would you place some code between starting some quickly terminated thread and setting/getting its priority, you would see same error on these platforms. Also ESRCH does not distinguish between outlived thread IDs and simply wrong ones, so passing any wrong value in FreeBSD will be silently ignored.
My suggestion is to add new parameter which will indicate whether druntime should ignore these errors and test in priority getter/setter whether a thread had terminated.

[MartinNowak]

My suggestion is to add new parameter which will indicate whether druntime should ignore these errors and test in priority getter/setter whether a thread had terminated.

We can't make setting/getting the priority of a thread an error without providing a way to synchronize that operation with the thread lifetime. The other problem is that access to m_isRunning is not synchronized so we're missing a way to determine whether a thread has terminated.

[complexmath]

Testing for whether a thread is running does have some potential slippage, but not much. Accesses to m_isRunning should probably be made atomic(msync.raw) at least just to make things more clear.

[braddr]

Essentially any use of a state variable in a conditional before taking some action is buggy unless both the conditional and the action are both done while a single lock is held throughout both operations. Just making the conditional check atomic is insufficient (unless it's one way, like isShutdown or similar conditions).

I totally agree that the conditional on freebsd part is fundamentally wrong. It's all platforms.

[complexmath]

I wouldn't make setting priority on a non-running thread an error because I'm inclined to say that you should be able to create a thread object, set its priority, and then start the thread. And now that I look at the code, it doesn't support this behavior. Either way though, I don't see a point in trying to enforce that this call is only valid for running threads.

[MartinNowak]

I totally agree that the conditional on freebsd part is fundamentally wrong. It's all platforms.

Well glibc and OSX's libc will simply set/get the priority on the dead thread.

Either way though, I don't see a point in trying to enforce that this call is only valid for running threads.

We'd need to save the priority in the Thread object.

I wouldn't make setting priority on a non-running thread an error because I'm inclined to say that you should be able to create a thread object, set its priority, and then start the thread.

Setting the priority before creation (in the pthread_attr_t) has no effect on FreeBSD. We tried that with #542.

[alexrp]

I'm inclined to agree with Sean. You should be able to create a thread, set its priority, start it, and have that priority take effect when it's started. If that means checking the priority in start and putting it into effect if it's non-default, then I think that's fine.

[braddr]

So, what are we going to do with this issue? I added a really fast mac to the build cluster some this weekend (can't stay full time). It failed something in the neighborhood of 1/4 of the test runs due to this bug.

[WalterBright]

I don't know enough about this issue to be able to pull this.

[MartinNowak]

Let's close it for now, people didn't like the idead of ignoring an error.
It seems that we can't reliably implement the current API, i.e. changing the priority of a running thread, because there is no simple way to know whether the thread represented by a Thread object has already terminated.

[MartinNowak]

Reopened.
I think we can be pragmatic about resolving this issue by ignoring this specific error, as invalid handles would only occur with memory corruption. I changed the documentation to reflect that behavior. The problem applies to all OSes btw.

[MartinNowak]

Implementation is now based on m_isRunning.

[quickfur]

ping
Looks like we have a merge conflict. Please rebase. I'd like to see this get merged to eliminate annoying random autotester failures.

[quickfur]

ping

[MartinNowak]

Updated

[quickfur]

Failing on Darwin/32 with "Unable to set priority". :-(

[MartinNowak]

OK updated again.

[quickfur]

Looks clean now. Time to merge?

[DmitryOlshansky]

LGTM

[DmitryOlshansky]

Auto-merge toggled on