Uh oh!
There was an error while loading. Please reload this page.
gh-125828 : Fix 'get_value' missing on [Bounded]Semaphore on multiprocessing MacOSX - #125829
gh-125828 : Fix 'get_value' missing on [Bounded]Semaphore on multiprocessing MacOSX#125829YvesDup wants to merge 22 commits into
Conversation
picnixz
left a comment
There was a problem hiding this comment.
Is there an alternative way to do it without a temporary class like this? (I don't have the answer but I feel it overcomplicates the code base)
| # Specific MacOSX Semaphore | ||
| # | ||
| class _MacOSXSemaphore(SemLock): |
There was a problem hiding this comment.
Wouldn't it be better to handle the SemLock class differently at the C level?
There was a problem hiding this comment.
Wouldn't it be better to handle the SemLock class differently at the C level?
As I mentioned in the problem, I'm waiting for feedback (from the Mac OS team ?) before going any further.
A C implementation is an option, even if it seems more complicated than Python.
That said, I'm available to go deep into the SemLock C code
There was a problem hiding this comment.
Actually, since the SemLock class is implemented in C, it's preferrable to first patch it there IMO. But let's CC some people I know they are on macOS: @ned-deily@ronaldoussoren@freakboy3742.
There was a problem hiding this comment.
I can't profess any particular expertise in this area. However, given the underlying implementation is written in C, I agree that it makes some sense for the implementation of this workaround to also be in C (rather than have an underlying implementation that returns NotImplemented, wrapped by a layer that overrides that.
There was a problem hiding this comment.
rather than have an underlying implementation that returns NotImplemented, wrapped by a layer that overrides that.
The main idea is to listen to the acquire and release methods to update a shared counter, and to return the counter value when the get_value method is invoked.
I am going to open a new PR with a workaround written in C.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
ZeroIntensity
commented
Oct 23, 2024
This needs a NEWS entry :) |
ronaldoussoren
commented
Oct 25, 2024
Is I haven't looked with enough detail at the code to be certain, and am not a multiprocessing expert, but I the code doesn't seem correct. In particular, how does the implementation handle blocking callers of acquire when the count is 0 but not when it is larger than 0? |
Yes, it is and I confirm this is not documented. This public method is rarely used to get a value of semaphore, except in unit tests. I should open an issue about this case.
As I explain above, this class only listen to In this way, the behaviour of semaphores is always done at a low level in the implementation of the |
* Refactor code * fix error on get_value, and nits
+ Add `pending_acquires` counter + Update comments + Fix nits
Documentation build overview
194 files changed · |
Clarified the fix for the unimplemented get_value method and specified the class hierarchy for better understanding.
This proposal is a workaround to fix absence of 'sem_getvalue' C function in the Semaphore MacOSX implementation.
Alls unit tests succeed except on
test.test_concurrent_futures.test_initrelative to the Resource Tracker manager.