Skip to content
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

bugfix: Use notify_all #162

Merged

Conversation

jdmoldenhauer
Copy link
Contributor

@jdmoldenhauer jdmoldenhauer commented Feb 14, 2022

The standard library threading.Condition class had notifyAll implemented as an alias to notify_all for backwards compatibility with older versions of python which still used the camel case methods. Python3.10 deprecated the notifyAll alias.

This change removes the call to notifyAll, in favor of notify_all which should exist as far back as python 3.5.

Requirements

  • I have added test coverage for new or changed functionality
    • Because the coverage report showed that the only line missing coverage in rwlock.py is line 41, and the only change is on line 32 it didn't seem like another test is needed.
    • Going to wait for one of the maintainers to let me know this okay.
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions
    • Ran the changes against python 3.6 thru 3.10. 3.5 wouldn't install. Hoping the travis CI run picks up 3.5.
    • CI picked up 3.5, and the tests ran successfully.

Related issues

None.

Describe the solution you've provided

Python 3.10 deprecated the backwards compatibility alias threading.Condition.notifyAll to notify_all. I've removed the call to notifyAll in favor of notify_all.

Describe alternatives you've considered

Considered doing something like this:

if hasattr(self._read_ready, 'notifyAll'):
    self._read_ready.notifyAll()
else:
    self._read_ready.notify_all()

However as notifyAll was only an alias for notify_all it didn't seem necessary given the support matrix for python beginning with 3.5. Additionally the docs for 3.5 have notify_all in them. So this conditional doesn't seem necessary.

Additional context

Changes are documented in the 3.10 release notes, and in an update to the documentation here.

The standard library threading.Condition class had `notifyAll`
implemented as an alias to `notify_all` for backwards comaptibility with
older versions of python which still used the camel case methods.
Python3.10 deprecated the `notifyAll` alias.

This change removes the call to `notifyAll`, in favor of `notify_all`
which should exist as far back as python 3.5.
@eli-darkly
Copy link
Contributor

Thanks for catching this. It is indeed an obsolete usage left over from the days when we had to support older Pythons.

@eli-darkly
Copy link
Contributor

(Filed internally as 142074)

@eli-darkly eli-darkly merged commit 2112623 into launchdarkly:master Feb 14, 2022
@jdmoldenhauer jdmoldenhauer deleted the bugfix/python310_notify_all branch February 14, 2022 18:12
LaunchDarklyReleaseBot pushed a commit that referenced this pull request Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants