Skip to content

bugfix: Use notify_all#162

Merged
eli-darkly merged 1 commit into
launchdarkly:masterfrom
jdmoldenhauer:bugfix/python310_notify_all
Feb 14, 2022
Merged

bugfix: Use notify_all#162
eli-darkly merged 1 commit into
launchdarkly:masterfrom
jdmoldenhauer:bugfix/python310_notify_all

Conversation

@jdmoldenhauer

@jdmoldenhauer jdmoldenhauer commented Feb 14, 2022

Copy link
Copy Markdown
Contributor

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

2 participants