No waiting on SemaphoreSlim #1936
Description
@denisz1 commented on Thu Jan 12 2017
For SemaphoreSlim, there are semaphore.Wait(), semaphore.Wait(int), semaphore.WaitAsync(), etc. methods.
Sometimes it is necessary to try obtain the semaphore, but if it is unavailable then not to wait. This is a real use case, as evident by the number of StackOverflow posts related to it.
So: semaphore.Wait(0) or semaphore.WaitAsync(0) or semaphore.Wait(TimeSpan.Zero), etc.
But that is undocumented. If this behavior is allowed, as I assume it is, please document it in the method notes. Until such time we cannot rely on this behavior.
@stephentoub commented on Thu Jan 12 2017
Wait/WaitAsync with a zero timeout will complete synchronously, either with a value of true if the semaphore could be acquired or a value of false if it would require waiting. This is supported. The docs could be improved to explicitly state this, but changing the behavior to anything else would break a bunch of code, including code in corefx itself, so even if it's not explicitly stated, you can certainly depend on it. Any other behavior would be a step backwards.
cc: @mairaw
@benaadams commented on Thu Jan 12 2017
The behaviour is correct (see: dotnet/coreclr#6898)
Are you suggesting a new api TryWait(...) and TryWaitAsync(...) to indicate it has a return? (Which would just map to Wait(...) and WaitAsync(...))
@stephentoub commented on Thu Jan 12 2017
Are you suggesting a new api
I believe they're just asking that the "completes synchronously and quickly" behavior be documented for an input of 0.
@karelz commented on Thu Jan 12 2017
If you can send me link to the docs which need to change and provide the new wording, I can ask docs team to make the changes.
@denisz1 would that be acceptable resolution of your issue?
@denisz1 commented on Thu Jan 12 2017
@stephentoub @benaadams @karelz No code change is necessary.
My point is that if it's not documented then we are not relying on formal behavior. So you could one day decide to change the behavior, and that would break our system.
For wording, I think there's something similar in Semaphore or WaitHandle? Otherwise something really really simple... it already says what milliseconds of -1, and >0 do, so just add one-liner for =0 option.
@JonHanna commented on Fri Jan 13 2017
I think the current behaviour is pretty much entailed by what is documented—a wait for zero milliseconds is no wait—but it wouldn't hurt to have it spelled out more explicitly. As well as making it clearer, it being pointed out could be somebody's "a ha! that's what I need!" moment.
@denisz1 commented on Fri Jan 13 2017
@JonHanna Maybe I'm not looking at the same docs as you, but it doesn't mention the zero case where I'm looking? Regardless, this is BCL stuff, so I agree with you that it should be "explicit".
@karelz commented on Fri Jan 13 2017
Can you guys each please link docs you're looking at? Ideally also suggest the wording change.
It will be easier to discuss it from there ...
@denisz1 commented on Fri Jan 13 2017
@karelz For Semaphore it says: If timeout is zero, the method does not block. It tests the state of the wait handle and returns immediately.
For SemaphoreSlim.WaitAsync and SemaphoreSlim.Wait it should say something similar / identical. There are a number of overloads, so they should all have the same wording.
Perhaps to make it clearer, something like "If the timeout is zero and the semaphore is unavailable, then the method does not block and returns immediately".
@karelz commented on Fri Jan 13 2017
I personally prefer the original wording - IMO it is clearer. I had to think about what does it mean "semaphore is unavailable" ... it sounds like "handle is closed" or something.
I am curious if WaitAsync(0) call basically makes the call synchronous. Is it worth calling out? (more for clarification and explanation what it does)
@stephentoub commented on Fri Jan 13 2017
I am curious if WaitAsync(0) call basically makes the call synchronous.
See the first sentence of my first reply above.
@karelz commented on Fri Jan 13 2017
Ah, missed that. And I see that you already looped in @mairaw ... thanks!
@mairaw commented on Fri Feb 10 2017
Can you guys please assign this to me? I'll review the bugs assigned to me here once I work through the backlog of items I have for the release. Thanks!
@karelz commented on Fri Feb 10 2017
@mairaw you don't have rights to modify issues? Let me fix that ...
@mairaw commented on Fri Feb 10 2017
Nope @karelz. Thanks!
@karelz commented on Fri Feb 10 2017
Now you do :)
Please check our triage rules - I guess [2] will have exception for documentation bugs which can be fixed only by your team ;-)