Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 73 additions & 5 deletions Lib/multiprocessing/synchronize.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,21 +125,89 @@ def _make_name():
return '%s-%s' % (process.current_process()._config['semprefix'],
next(SemLock._rand))

if sys.platform == 'darwin':
#
# Specific MacOSX Semaphore
#

class _MacOSXSemaphore(SemLock):
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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 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.

"""Dedicated class used only to workaround the missing
function 'sem_getvalue', when interpreter runs on MacOSX.
Add a shared counter for each [Bounded]Semaphore in order
to handle internal counter when acquire and release operations
are called.
"""

def __init__(self, kind, value, maxvalue, *, ctx):
util.debug(f"_MacOSXSemaphore:: creation of a {self.__class__.__name__}"\
f"with '{value = }'")
Comment on lines +142 to +143
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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}")

SemLock.__init__(self, kind, value, maxvalue, ctx=ctx)
self._count = ctx.Value('L', value) # May be more than 'L' ?

def _acquire(self, *args, **kwargs) -> bool:
if self._semlock.acquire(*args, **kwargs):
with self._count:
util.debug(f"_MacOSXSemaphore: acquire {repr(self)}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
util.debug(f"_MacOSXSemaphore: acquire {repr(self)}")
util.debug(f"_MacOSXSemaphore: acquire {self!r}")

self._count.value -= 1
return True
return False

def _release(self):
with self._count:
self._count.value += 1
self._semlock.release()
util.debug(f"_MacOSXSemaphore: release {repr(self)}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
util.debug(f"_MacOSXSemaphore: release {repr(self)}")
util.debug(f"_MacOSXSemaphore: release {self!r}")


def _release_bounded(self):
if self._count.value + 1 > self._semlock.maxvalue:
raise ValueError(f"Cannot exceed initial value of"\
f" {self._semlock.maxvalue!a}")
self._release()

def _get_value(self) -> int:
return self._count.value

def _make_methods(self):
super()._make_methods()
util.debug("_MacOSXSemaphore: _make_methods call")
self.acquire = self._acquire
if isinstance(self, BoundedSemaphore):
self.release = self._release_bounded
elif isinstance(self, Semaphore):
self.release = self._release
else:
raise RuntimeError("Class dedicated only to Semaphore or BoundedSemaphore OSX")
self.get_value = self._get_value

def __setstate__(self, state):
self._count, state = state[-1], state[:-1]
super().__setstate__(state)

def __getstate__(self) -> tuple:
return super().__getstate__() + (self._count,)


_SemClass = _MacOSXSemaphore
else:
_SemClass = SemLock

#
# Semaphore
#

class Semaphore(SemLock):
class Semaphore(_SemClass):

def __init__(self, value=1, *, ctx):
SemLock.__init__(self, SEMAPHORE, value, SEM_VALUE_MAX, ctx=ctx)
_SemClass.__init__(self, SEMAPHORE, value, SEM_VALUE_MAX, ctx=ctx)

def get_value(self):
"""redefined when MacOSX.
"""
return self._semlock._get_value()

def __repr__(self):
try:
value = self._semlock._get_value()
value = self.get_value()
except Exception:
value = 'unknown'
return '<%s(value=%s)>' % (self.__class__.__name__, value)
Expand All @@ -151,11 +219,11 @@ def __repr__(self):
class BoundedSemaphore(Semaphore):

def __init__(self, value=1, *, ctx):
SemLock.__init__(self, SEMAPHORE, value, value, ctx=ctx)
_SemClass.__init__(self, SEMAPHORE, value, value, ctx=ctx)

def __repr__(self):
try:
value = self._semlock._get_value()
value = self.get_value()
except Exception:
value = 'unknown'
return '<%s(value=%s, maxvalue=%s)>' % \
Expand Down
24 changes: 20 additions & 4 deletions Lib/test/_test_multiprocessing.py
Original file line number Diff line number Diff line change
Expand Up @@ -1664,10 +1664,26 @@ def test_semaphore(self):
def test_bounded_semaphore(self):
sem = self.BoundedSemaphore(2)
self._test_semaphore(sem)
# Currently fails on OS/X
#if HAVE_GETVALUE:
# self.assertRaises(ValueError, sem.release)
# self.assertReturnsIfImplemented(2, get_value, sem)
self.assertRaises(ValueError, sem.release)
self.assertReturnsIfImplemented(2, get_value, sem)

@unittest.skipIf(sys.platform != 'darwin', 'Darwin only')
def test_detect_macosx_semaphore(self):
if self.TYPE != 'processes':
self.skipTest('test not appropriate for {}'.format(self.TYPE))

sem = self.Semaphore(2)
mro = sem.__class__.mro()
self.assertTrue(any('_MacOSXSemaphore' in cls.__name__ for cls in mro))

@unittest.skipIf(sys.platform != 'darwin', 'Darwin only')
def test_detect_macosx_boundedsemaphore(self):
if self.TYPE != 'processes':
self.skipTest('test not appropriate for {}'.format(self.TYPE))

sem = self.BoundedSemaphore(2)
mro = sem.__class__.mro()
self.assertTrue(any('_MacOSXSemaphore' in cls.__name__ for cls in mro))

def test_timeout(self):
if self.TYPE != 'processes':
Expand Down
Loading