python-zk / kazoo

Kazoo is a high-level Python library that makes it easier to use Apache Zookeeper.
https://kazoo.readthedocs.io
Apache License 2.0
1.3k stars 387 forks source link

fix(core): use selectors to poll connections instead of raw select in threading,gevent,eventlet; solve the select limitation on a maximum file handler value and dynamic choose the best poller in the system #656

Closed JetDrag closed 2 years ago

JetDrag commented 3 years ago

Use selectors to poll connections instead of raw select in all kinds of handlers. To solve the limitation on a maximum file handler value and dynamic choose the best poller in the system.

Why is this needed?

In the current implementation, the gevent, eventlet handler use the old select fuction which not support fd number langer than 1023 . Network Program with high numbers of opened sockets often raise Exception: filedescriptor out of range in select()

Proposed Changes

JetDrag commented 3 years ago

Hi, @ceache I've reslove the all discussion through adding some fix and testcase. Please review again.

StephenSorriaux commented 2 years ago

Hello, yes I am! Thanks a lot for this.

JetDrag commented 2 years ago

Hi, @StephenSorriaux @ceache do you have time to merge the PR and release a new version? So we don't have to deploy a custom version in out production environment.

StephenSorriaux commented 2 years ago

Hello @JetDrag, can you please rebase your branch with the current master so that we can check all tests are passing? Sorry for the delay, we were in the process of updating our CI.

codecov-commenter commented 2 years ago

Codecov Report

Merging #656 (b340e38) into master (f585d60) will increase coverage by 0.22%. The diff coverage is 93.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #656      +/-   ##
==========================================
+ Coverage   94.21%   94.43%   +0.22%     
==========================================
  Files          56       57       +1     
  Lines        8246     8342      +96     
==========================================
+ Hits         7769     7878     +109     
+ Misses        477      464      -13     
Impacted Files Coverage Δ
kazoo/tests/test_eventlet_handler.py 89.61% <80.00%> (-0.57%) :arrow_down:
kazoo/tests/test_gevent_handler.py 78.16% <85.00%> (+2.75%) :arrow_up:
kazoo/handlers/utils.py 93.24% <95.55%> (+10.19%) :arrow_up:
kazoo/tests/test_selectors_select.py 97.01% <97.01%> (ø)
kazoo/handlers/eventlet.py 100.00% <100.00%> (+1.98%) :arrow_up:
kazoo/handlers/gevent.py 94.62% <100.00%> (+1.21%) :arrow_up:
kazoo/handlers/threading.py 87.23% <100.00%> (-4.05%) :arrow_down:
kazoo/tests/test_threading_handler.py 96.18% <100.00%> (+0.29%) :arrow_up:
kazoo/tests/test_cache.py 58.16% <0.00%> (-0.69%) :arrow_down:
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f585d60...b340e38. Read the comment docs.

ceache commented 2 years ago

Hi, it seems to have failed

FAILED kazoo/tests/test_selectors_select.py::SelectTestCase::test_select_mutated

Cheers,

JetDrag commented 2 years ago

@ceache sorry for the fail. I've update the test case, approval the flow again pls.

JetDrag commented 2 years ago

@ceache The test cases passed in the official python version, but all greenlet related test case failed in the PyPy. It seems that the greenlet don't support the PyPy.

StephenSorriaux commented 2 years ago

Can it be linked with the new test_huge_file_descriptor which creates lots of sockets that, if I am not wrong, are never closed?

JetDrag commented 2 years ago

@StephenSorriaux I've updated the test cases and run in local PyPy env,I think you are correct. Pls approval the flow again .

JetDrag commented 2 years ago

It seems that the pr matched the condition to merge already. Could you merge the PR and release a new version, many thanks! @ceache @StephenSorriaux

JetDrag commented 2 years ago

Nice to see you in new year~

It would be a good choice to release a new version at the beginning of 2022 to fix this general problem. The modification of this PR has been validated in our production environment for months and reliable enough.

Cheers