Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-125828 : Fix 'get_value' missing on [Bounded]Semaphore on multiprocessing MacOSX#125829
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Wouldn't it be better to handle the SemLock class differently at the C level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
rather than have an underlying implementation that returns NotImplemented, wrapped by a layer that overrides that.
The main idea is to listen to theacquire andrelease methods to update a shared counter, and to return the counter value when theget_value method is invoked.
I am going to open a new PR with a workaround written in C.
| util.debug(f"_MacOSXSemaphore:: creation of a {self.__class__.__name__}"\ | ||
| f"with '{value = }'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| util.debug(f"_MacOSXSemaphore:: creation of a{self.__class__.__name__}"\ | |
| f"with'{value=}'") | |
| util.debug(f"_MacOSXSemaphore:: creation of a{self.__class__.__name__}"\ | |
| f"with{value=!r}") |
| def _acquire(self, *args, **kwargs) -> bool: | ||
| if self._semlock.acquire(*args, **kwargs): | ||
| with self._count: | ||
| util.debug(f"_MacOSXSemaphore: acquire {repr(self)}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| util.debug(f"_MacOSXSemaphore: acquire{repr(self)}") | |
| util.debug(f"_MacOSXSemaphore: acquire{self!r}") |
| with self._count: | ||
| self._count.value += 1 | ||
| self._semlock.release() | ||
| util.debug(f"_MacOSXSemaphore: release {repr(self)}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| util.debug(f"_MacOSXSemaphore: release{repr(self)}") | |
| util.debug(f"_MacOSXSemaphore: release{self!r}") |
This needs a NEWS entry :) |
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? |
YvesDup commentedNov 4, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 itabove, this class only listen to In this way, the behaviour of semaphores is always done at a low level in the implementation of the |
Uh oh!
There was an error while loading.Please reload this page.
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.