Repository: python-zk/kazoo
Pull request #681 (2022-11-04 19:17:06)

fix(core): fix for Windows platform
Fixes https://github.com/python-zk/kazoo/issues/679

## Why is this needed?
This fixes an issue with the Windows platform that appeared in the latest 2.9.0 release. This also adds a very basic testing for the Windows platform (only 1 job, for the latest version of Python and the latest version of ZK).

## Proposed Changes
  - remove the `os.fstat(fd)` check that is invalid on Windows platform
  - adjust `test_selector_select` test
  - add a single job to test Windows platform (`gevent` seems broken on Windows so... skipped it): only 1 job because I don't feel like we need to re-test every version of Python/ZK for Windows

## Does this PR introduce any breaking change?
Nah.


ceache: @StephenSorriaux  is this a draft or ready for review yet?
StephenSorriaux: @ceache Not really, it is still a draft. I'm just too used to gitlab, which auto marks a PR as draft if "draft" is in the title.
jeffwidman: What's needed to move this forward? It'd be nice to get this resolved for our Windows users.
StephenSorriaux: I was waiting for feedbacks from @JetDrag / @ceache but I will go ahead and just finish it so that it can be reviewed.
codecov[bot]: ## [Codecov](https://codecov.io/gh/python-zk/kazoo/pull/681?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-zk) Report
Patch coverage: **`71.42`**% and project coverage change: **`-0.20`** :warning:
> Comparison is base [(`2c36d69`)](https://codecov.io/gh/python-zk/kazoo/commit/2c36d699fbf0569f490d5bfa66fa85fbdc68dea3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-zk) 96.65% compared to head [(`3d620b3`)](https://codecov.io/gh/python-zk/kazoo/pull/681?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-zk) 96.45%.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master     #681      +/-   ##
==========================================
- Coverage   96.65%   96.45%   -0.20%     
==========================================
  Files          27       27              
  Lines        3554     3557       +3     
==========================================
- Hits         3435     3431       -4     
- Misses        119      126       +7     
```


| [Impacted Files](https://codecov.io/gh/python-zk/kazoo/pull/681?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-zk) | Coverage Δ | |
|---|---|---|
| [kazoo/recipe/lock.py](https://codecov.io/gh/python-zk/kazoo/pull/681?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-zk#diff-a2F6b28vcmVjaXBlL2xvY2sucHk=) | `99.64% <ø> (+0.35%)` | :arrow_up: |
| [kazoo/handlers/utils.py](https://codecov.io/gh/python-zk/kazoo/pull/681?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-zk#diff-a2F6b28vaGFuZGxlcnMvdXRpbHMucHk=) | `95.49% <66.66%> (+0.97%)` | :arrow_up: |
| [kazoo/protocol/connection.py](https://codecov.io/gh/python-zk/kazoo/pull/681?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-zk#diff-a2F6b28vcHJvdG9jb2wvY29ubmVjdGlvbi5weQ==) | `96.28% <100.00%> (-1.45%)` | :arrow_down: |

... and [4 files with indirect coverage changes](https://codecov.io/gh/python-zk/kazoo/pull/681/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-zk)

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-zk). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-zk)

</details>

[:umbrella: View full report in Codecov by Sentry](https://codecov.io/gh/python-zk/kazoo/pull/681?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-zk).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-zk).

JetDrag: Excellent work！ @StephenSorriaux 
StephenSorriaux: @ceache @jeffwidman Sorry to resurrect this only now, would appreciate another review if possible and finally merge this.

Patch:
diff --git a/kazoo/handlers/utils.py b/kazoo/handlers/utils.py
index 2abe1ff..858df38 100644
--- a/kazoo/handlers/utils.py
+++ b/kazoo/handlers/utils.py
@@ -3,7 +3,6 @@
 from collections import defaultdict
 import errno
 import functools
-import os
 import select
 import selectors
 import ssl
@@ -349,7 +348,6 @@ def fileobj_to_fd(fileobj):
             raise TypeError("Invalid file object: " "{!r}".format(fileobj))
     if fd < 0:
         raise TypeError("Invalid file descriptor: {}".format(fd))
-    os.fstat(fd)
     return fd
 
 
@@ -380,7 +378,11 @@ def selector_select(
 
     selector = selectors_module.DefaultSelector()
     for fd, events in fd_events.items():
-        selector.register(fd, events)
+        try:
+            selector.register(fd, events)
+        except (ValueError, OSError) as e:
+            # gevent can raise OSError
+            raise ValueError("Invalid event mask or fd") from e
 
     revents, wevents, xevents = [], [], []
     try:
diff --git a/kazoo/recipe/lock.py b/kazoo/recipe/lock.py
index 59c6d8f..8a49039 100644
--- a/kazoo/recipe/lock.py
+++ b/kazoo/recipe/lock.py
@@ -209,7 +209,6 @@ class Lock(object):
         return True
 
     def _inner_acquire(self, blocking, timeout, ephemeral=True):
-
         # wait until it's our chance to get it..
         if self.is_acquired:
             if not blocking:
diff --git a/tox.ini b/tox.ini
index 7a6cfe0..567acd2 100644
--- a/tox.ini
+++ b/tox.ini
@@ -32,7 +32,9 @@ deps =
 allowlist_externals =
     {toxinidir}/ensure-zookeeper-env.sh
     {toxinidir}/init_krb5.sh
+    bash
 commands =
+        bash \
     sasl: {toxinidir}/init_krb5.sh {envtmpdir}/kerberos \
         {toxinidir}/ensure-zookeeper-env.sh \
         pytest {posargs: -ra -v --cov-report=xml --cov=kazoo kazoo/tests}


Test patch:
diff --git a/.github/workflows/testing.yml b/.github/workflows/testing.yml
index bab6b8a..f169e3e 100644
--- a/.github/workflows/testing.yml
+++ b/.github/workflows/testing.yml
@@ -17,11 +17,9 @@ jobs:
     runs-on: ubuntu-latest
 
     steps:
-      - name: Handle the code
-        uses: actions/checkout@v4
+      - uses: actions/checkout@v4
 
-      - name: "Set up Python 3.10"
-        uses: actions/setup-python@v4
+      - uses: actions/setup-python@v4
         with:
           python-version: "3.10"
 
@@ -47,9 +45,8 @@ jobs:
     needs: [validate]
 
     name: >
-      Test Python ${{ matrix.python-version }},
+      Linux - Test Python ${{ matrix.python-version }},
       ZK ${{ matrix.zk-version }}
-
     runs-on: ubuntu-latest
 
     strategy:
@@ -69,8 +66,7 @@ jobs:
           - python-version: "pypy-3.7"
             tox-env: pypy3
     steps:
-      - name: Handle the code
-        uses: actions/checkout@v4
+      - uses: actions/checkout@v4
 
       - name: Set up Python ${{ matrix.python-version }}
         uses: actions/setup-python@v4
@@ -111,3 +107,52 @@ jobs:
 
       - name: Publish Codecov report
         uses: codecov/codecov-action@v3
+
+  test_windows:
+    needs: [validate]
+    name: Windows - Sanity test using a single version of Python and ZK
+
+    runs-on: windows-latest
+    steps:
+      - uses: actions/checkout@v4
+
+      - uses: actions/setup-python@v4
+        with:
+          python-version: "3.10"
+
+      - name: Handle pip cache
+        uses: actions/cache@v3
+        with:
+          path: ~/.cache/pip
+          key: ${{ runner.os }}-pip-${{ hashFiles('**/requirements*.txt') }}
+          restore-keys: |
+            ${{ runner.os }}-pip-
+
+      - name: Handle ZK installation cache
+        uses: actions/cache@v3
+        with:
+          path: zookeeper
+          key: ${{ runner.os }}-zookeeper
+          restore-keys: |
+            ${{ runner.os }}-zookeeper
+
+      # https://github.com/actions/setup-java
+      - name: Setup Java
+        uses: actions/setup-java@v3
+        with:
+          distribution: 'temurin'
+          java-version: '17'
+
+      - name: Install required dependencies
+        run: |
+          python3 -m pip install --upgrade pip
+          pip install tox
+
+      - name: Test with tox
+        run: tox -e py310
+        env:
+          ZOOKEEPER_VERSION: 3.7.1
+          ZOOKEEPER_PREFIX: "apache-"
+          ZOOKEEPER_SUFFIX: "-bin"
+          ZOOKEEPER_LIB: "lib"
+        shell: bash
diff --git a/kazoo/testing/common.py b/kazoo/testing/common.py
index a349199..3c635ef 100644
--- a/kazoo/testing/common.py
+++ b/kazoo/testing/common.py
@@ -240,7 +240,7 @@ log4j.appender.ROLLINGFILE.File="""
         jars = glob((os.path.join(self.install_path, "zookeeper-*.jar")))
         if jars:
             # Release build (`ant package`)
-            jars.extend(glob(os.path.join(self.install_path, "lib/*.jar")))
+            jars.extend(glob(os.path.join(self.install_path, "lib", "*.jar")))
             jars.extend(glob(os.path.join(self.install_path, "*.jar")))
             # support for different file locations on Debian/Ubuntu
             jars.extend(glob(os.path.join(self.install_path, "log4j-*.jar")))
@@ -253,10 +253,10 @@ log4j.appender.ROLLINGFILE.File="""
         else:
             # Development build (plain `ant`)
             jars = glob(
-                (os.path.join(self.install_path, "build/zookeeper-*.jar"))
+                (os.path.join(self.install_path, "build", "zookeeper-*.jar"))
             )
             jars.extend(
-                glob(os.path.join(self.install_path, "build/lib/*.jar"))
+                glob(os.path.join(self.install_path, "build", "lib", "*.jar"))
             )
 
         return os.pathsep.join(jars)
diff --git a/kazoo/tests/test_cache.py b/kazoo/tests/test_cache.py
index 31dabc7..7251db6 100644
--- a/kazoo/tests/test_cache.py
+++ b/kazoo/tests/test_cache.py
@@ -1,5 +1,6 @@
 import gc
 import importlib
+import sys
 import uuid
 
 from unittest.mock import patch, call, Mock
@@ -28,6 +29,11 @@ class KazooAdaptiveHandlerTestCase(KazooTestHarness):
 
     def choose_an_installed_handler(self):
         for handler_module, handler_class in self.HANDLERS:
+            if (
+                handler_module == "kazoo.handlers.gevent"
+                and sys.platform == "win32"
+            ):
+                continue
             try:
                 mod = importlib.import_module(handler_module)
                 cls = getattr(mod, handler_class)
diff --git a/kazoo/tests/test_eventlet_handler.py b/kazoo/tests/test_eventlet_handler.py
index 5e89789..2a201a8 100644
--- a/kazoo/tests/test_eventlet_handler.py
+++ b/kazoo/tests/test_eventlet_handler.py
@@ -131,7 +131,10 @@ class TestEventletHandler(unittest.TestCase):
             r.get()
 
     def test_huge_file_descriptor(self):
-        import resource
+        try:
+            import resource
+        except ImportError:
+            self.skipTest("resource module unavailable on this platform")
         from eventlet.green import socket
         from kazoo.handlers.utils import create_tcp_socket
 
diff --git a/kazoo/tests/test_gevent_handler.py b/kazoo/tests/test_gevent_handler.py
index 0745c06..fb37238 100644
--- a/kazoo/tests/test_gevent_handler.py
+++ b/kazoo/tests/test_gevent_handler.py
@@ -1,4 +1,5 @@
 import unittest
+import sys
 
 import pytest
 
@@ -9,6 +10,7 @@ from kazoo.testing import KazooTestCase
 from kazoo.tests import test_client
 
 
+@pytest.mark.skipif(sys.platform == "win32", reason="does not run on windows")
 class TestGeventHandler(unittest.TestCase):
     def setUp(self):
         try:
@@ -77,6 +79,7 @@ class TestGeventHandler(unittest.TestCase):
         ev.wait()
 
 
+@pytest.mark.skipif(sys.platform == "win32", reason="does not run on windows")
 class TestBasicGeventClient(KazooTestCase):
     def setUp(self):
         try:
@@ -165,6 +168,7 @@ class TestBasicGeventClient(KazooTestCase):
             sock.close()
 
 
+@pytest.mark.skipif(sys.platform == "win32", reason="does not run on windows")
 class TestGeventClient(test_client.TestClient):
     def setUp(self):
         try:
diff --git a/kazoo/tests/test_hosts.py b/kazoo/tests/test_hosts.py
index 8f8a811..a2fae1a 100644
--- a/kazoo/tests/test_hosts.py
+++ b/kazoo/tests/test_hosts.py
@@ -36,7 +36,6 @@ class HostsTestCase(TestCase):
         assert chroot is None
 
     def test_hosts_list(self):
-
         hosts, chroot = collect_hosts("zk01:2181, zk02:2181, zk03:2181")
         expected1 = [("zk01", 2181), ("zk02", 2181), ("zk03", 2181)]
         assert hosts == expected1
diff --git a/kazoo/tests/test_selectors_select.py b/kazoo/tests/test_selectors_select.py
index 19b224d..99dd44a 100644
--- a/kazoo/tests/test_selectors_select.py
+++ b/kazoo/tests/test_selectors_select.py
@@ -3,7 +3,6 @@ The official python select function test case copied from python source
  to test the selector_select function.
 """
 
-import errno
 import os
 import socket
 import sys
@@ -41,12 +40,7 @@ class SelectTestCase(unittest.TestCase):
         with open(__file__, "rb") as fp:
             fd = fp.fileno()
             fp.close()
-            try:
-                select([fd], [], [], 0)
-            except OSError as err:
-                self.assertEqual(err.errno, errno.EBADF)
-            else:
-                self.fail("exception not raised")
+            self.assertRaises(ValueError, select, [fd], [], [], 0)
 
     def test_returned_list_identity(self):
         # See issue #8329
diff --git a/kazoo/tests/test_threading_handler.py b/kazoo/tests/test_threading_handler.py
index 664c444..7799787 100644
--- a/kazoo/tests/test_threading_handler.py
+++ b/kazoo/tests/test_threading_handler.py
@@ -45,7 +45,10 @@ class TestThreadingHandler(unittest.TestCase):
         assert h._running is False
 
     def test_huge_file_descriptor(self):
-        import resource
+        try:
+            import resource
+        except ImportError:
+            self.skipTest("resource module unavailable on this platform")
         import socket
         from kazoo.handlers.utils import create_tcp_socket
 
diff --git a/kazoo/tests/util.py b/kazoo/tests/util.py
index 6351468..836775a 100644
--- a/kazoo/tests/util.py
+++ b/kazoo/tests/util.py
@@ -98,7 +98,6 @@ class Wait(object):
         getnow=(lambda: time.time),
         getsleep=(lambda: time.sleep),
     ):
-
         if timeout is not None:
             self.timeout = timeout