fix(core): fix for Windows platform #681

StephenSorriaux · 2022-11-04T19:17:06Z

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

kazoo/tests/test_gevent_handler.py

kazoo/handlers/utils.py

StephenSorriaux · 2022-11-14T20:53:02Z

kazoo/handlers/utils.py

+    # XXX: needed?
+    # selectors_module.register handle fileobject, do we need the use of fileobj_to_fd()?
+    # is this test valid for Windows?
+    # os.fstat(fd)


Hello @JetDrag,

Sorry to summon you here. This os.fstat(fd) call is generating crashes on Windows because the handle is, it seems, invalid. With this call commented out, the crash does not appear (and tests are running fine).
Since you did this implementation, I was wondering if:

there was a reason using the fileobj_to_fd() method instead of directly giving the fileobj to selectors_module.register() (which seems to be capable of handling fileobj directly)?

you know if the os.fstat(fd) test is valid for Windows?

Thank you.

I second this.
This os.fstat() does not do anything.
Actually, python3 handles os.stat() on FD or file just fine.

https://docs.python.org/3/library/os.html#os.stat

Hi, @StephenSorriaux @ceache

The function fileobj_to_fd is same as _fileobj_to_fd function in selectors module almost except the last second line os.fstat(fd).

I did this implementation to simulate the raw select behavior via test case test_selectors_select, which is modified from Python official test cast test_select.

If I don't do that, the test cast test_errno in the test_selectors_select will fail, because the exception (OSError with errno 22, EINVAL) raise by some selectors.BaseSelector implementations don't meet the test case requirement：raise OSError with errno 9 (EBADF) when select meet a closed fd.

Maybe we cloud make some other simulate implementation to make those spec working on the Windows?

https://github.com/python/cpython/blob/main/Lib/test/test_select.py#L32 test_errno() for reference.

I do not think we care about this behavior in Kazoo.
Actually, the err conditions with selector when selecting "bad file descriptors" are varied depending on platform and selector implementations (there are threads about this on the Python bug report).
This is here trying to make sure that selectors implementations are all equivalente to select() behavior and we really can't do that.

IMHO. the only thing we need to do is ensure how Kazoo reacts to a closed FD.
In other words, be it EINVAL or EBADF or other, we should raise an uniform Kazoo error (with Python3, we could pass up the original, OS/Implementation dependent Exception).

What do you think?

It maybe the best solution.

Thank you both for your thoughts. As suggested, I tried to unify the exceptions that can be raised and catch that in the test_errno() testcase. That may be too naive, not sure really, so I would love your feedback on this.

ceache · 2022-11-16T22:41:24Z

@StephenSorriaux is this a draft or ready for review yet?

StephenSorriaux · 2022-11-16T23:13:53Z

@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 · 2023-01-29T02:42:39Z

What's needed to move this forward? It'd be nice to get this resolved for our Windows users.

StephenSorriaux · 2023-01-31T18:42:11Z

I was waiting for feedbacks from @JetDrag / @ceache but I will go ahead and just finish it so that it can be reviewed.

ceache

Thanks!

jeffwidman

Looks fine to me, with the caveat that I haven't actually written/run python on 🪟 in years.

.github/workflows/testing.yml

kazoo/handlers/utils.py

JetDrag · 2023-05-25T12:26:45Z

Excellent work！ @StephenSorriaux

Performing an `os.stat(fd)` on Windows platform generate a crash because the "handler is not valid". `select` already handle this possible case and there is no actual need to perform any kind of check before. This was removed, and the linked test slightly changed.

StephenSorriaux · 2024-01-19T21:09:08Z

@ceache @jeffwidman Sorry to resurrect this only now, would appreciate another review if possible and finally merge this.

jeffwidman

LGTM, with the caveat that I don't run Windows. But I think the additional confidence of the basic sanity test is enough to be comfortable merging this.

Thank you for your hard work on this!

StephenSorriaux · 2024-01-19T21:22:20Z

Thanks for always being around my dear Jeff!

StephenSorriaux force-pushed the feat/windows-testing branch 15 times, most recently from 0495776 to c901aaa Compare November 11, 2022 04:13

houndci-bot reviewed Nov 11, 2022

View reviewed changes

kazoo/tests/test_gevent_handler.py Show resolved Hide resolved

kazoo/tests/test_gevent_handler.py Show resolved Hide resolved

StephenSorriaux force-pushed the feat/windows-testing branch 2 times, most recently from 9e217a7 to ae07056 Compare November 11, 2022 04:39

StephenSorriaux mentioned this pull request Nov 14, 2022

Kazoo 2.9.0 throws "Connection dropped: socket connection error: The handle is invalid" #679

Closed

StephenSorriaux force-pushed the feat/windows-testing branch from ae07056 to 6364a21 Compare November 14, 2022 20:48

houndci-bot reviewed Nov 14, 2022

View reviewed changes

kazoo/handlers/utils.py Outdated Show resolved Hide resolved

StephenSorriaux commented Nov 14, 2022

View reviewed changes

StephenSorriaux marked this pull request as draft November 16, 2022 23:12

StephenSorriaux force-pushed the feat/windows-testing branch from 6364a21 to 5f70985 Compare November 28, 2022 21:59

StephenSorriaux force-pushed the feat/windows-testing branch 2 times, most recently from 3ff9d53 to 8c747d4 Compare April 24, 2023 18:21

StephenSorriaux force-pushed the feat/windows-testing branch from d462faa to 623bf83 Compare April 24, 2023 22:13

StephenSorriaux marked this pull request as ready for review April 24, 2023 23:42

StephenSorriaux requested review from jeffwidman, ceache and a-ungurianu April 24, 2023 23:42

StephenSorriaux mentioned this pull request May 24, 2023

New kazoo release to include SSL fixes #720

Closed

StephenSorriaux requested review from jeffwidman, ceache and a-ungurianu and removed request for jeffwidman, ceache and a-ungurianu May 24, 2023 13:02

ceache previously approved these changes May 24, 2023

View reviewed changes

jeffwidman previously approved these changes May 24, 2023

View reviewed changes

StephenSorriaux force-pushed the feat/windows-testing branch from 623bf83 to f72b19b Compare January 19, 2024 19:57

StephenSorriaux dismissed stale reviews from ceache and jeffwidman via d5dae3b January 19, 2024 20:11

StephenSorriaux force-pushed the feat/windows-testing branch 2 times, most recently from d5dae3b to d3a688a Compare January 19, 2024 20:13

StephenSorriaux added 2 commits January 19, 2024 15:35

feat(testing): add test for Windows platform

e5becc3

StephenSorriaux force-pushed the feat/windows-testing branch from d3a688a to e5becc3 Compare January 19, 2024 20:37

StephenSorriaux requested review from jeffwidman and ceache January 19, 2024 21:08

jeffwidman approved these changes Jan 19, 2024

View reviewed changes

StephenSorriaux merged commit 31fdca6 into master Jan 19, 2024
26 checks passed

StephenSorriaux deleted the feat/windows-testing branch January 19, 2024 21:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): fix for Windows platform #681

fix(core): fix for Windows platform #681

StephenSorriaux commented Nov 4, 2022 •

edited

Loading

StephenSorriaux Nov 14, 2022

ceache Nov 15, 2022

ceache Nov 15, 2022

JetDrag Nov 15, 2022 •

edited

Loading

ceache Nov 16, 2022

JetDrag Nov 17, 2022

StephenSorriaux Nov 28, 2022

ceache commented Nov 16, 2022

StephenSorriaux commented Nov 16, 2022

jeffwidman commented Jan 29, 2023

StephenSorriaux commented Jan 31, 2023

ceache left a comment

jeffwidman left a comment

JetDrag commented May 25, 2023

StephenSorriaux commented Jan 19, 2024

jeffwidman left a comment

StephenSorriaux commented Jan 19, 2024

fix(core): fix for Windows platform #681

fix(core): fix for Windows platform #681

Conversation

StephenSorriaux commented Nov 4, 2022 • edited Loading

Why is this needed?

Proposed Changes

Does this PR introduce any breaking change?

StephenSorriaux Nov 14, 2022

Choose a reason for hiding this comment

ceache Nov 15, 2022

Choose a reason for hiding this comment

ceache Nov 15, 2022

Choose a reason for hiding this comment

JetDrag Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

ceache Nov 16, 2022

Choose a reason for hiding this comment

JetDrag Nov 17, 2022

Choose a reason for hiding this comment

StephenSorriaux Nov 28, 2022

Choose a reason for hiding this comment

ceache commented Nov 16, 2022

StephenSorriaux commented Nov 16, 2022

jeffwidman commented Jan 29, 2023

StephenSorriaux commented Jan 31, 2023

ceache left a comment

Choose a reason for hiding this comment

jeffwidman left a comment

Choose a reason for hiding this comment

JetDrag commented May 25, 2023

StephenSorriaux commented Jan 19, 2024

jeffwidman left a comment

Choose a reason for hiding this comment

StephenSorriaux commented Jan 19, 2024

StephenSorriaux commented Nov 4, 2022 •

edited

Loading

JetDrag Nov 15, 2022 •

edited

Loading