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

Use poll() instead of select(), unless Windows. #2865

Merged
merged 2 commits into from Apr 21, 2023

Conversation

I-question-this
Copy link
Contributor

Fixes #2278, which was originally addressed in #2279, but was not
properly merged. Additionally it did not address the problem
of poll not existing on Windows. This patch falls back on the
more limited select method if host system is Windows.

Signed-off-by: Tyler Westland tylerofthewest@gmail.com

Fixes docker#2278, which was originally addressed in docker#2279, but was not
properly merged. Additionally it did not address the problem
of poll not existing on Windows. This patch falls back on the
more limited select method if host system is Windows.

Signed-off-by: Tyler Westland <tylerofthewest@gmail.com>
@joaoleveiga
Copy link

Hello there. Is there a chance that this PR can be picked up again? As it stands it looks like it's incorrectly signed off: https://github.com/docker/docker-py/pull/2865/checks?check_run_id=4356046759

Can you do this, @I-question-this or might someone else pick it up?

Thanks

@I-question-this
Copy link
Contributor Author

Hello there. Is there a chance that this PR can be picked up again? As it stands it looks like it's incorrectly signed off: https://github.com/docker/docker-py/pull/2865/checks?check_run_id=4356046759

Can you do this, @I-question-this or might someone else pick it up?

Thanks

@jjoaoleveiga I have resolved this issue. I had not intended for that second commit to be a part of this but had not properly made a different branch.

@milas milas requested review from milas and removed request for ulyssessouza and aiordache April 21, 2023 21:12
@milas milas self-assigned this Apr 21, 2023
Copy link
Member

@milas milas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! CI looks good on Linux, and I verified it hits the select path on Windows and poll on macOS.

@milas milas merged commit a02ba74 into docker:main Apr 21, 2023
10 checks passed
felixfontein added a commit to felixfontein/community.docker that referenced this pull request May 10, 2023
…er-py#2865)

Fixes docker/docker-py#2278, which was originally addressed in docker/docker-py#2279, but was not
properly merged. Additionally it did not address the problem
of poll not existing on Windows. This patch falls back on the
more limited select method if host system is Windows.

Cherry-picked from docker/docker-py@a02ba74

Co-authored-by: Tyler Westland <tylerofthewest@gmail.com>
tito added a commit to tito/docker-py that referenced this pull request May 16, 2023
…form check

The implementation done in docker#2865 is breaking usage of docker-py library within eventlet.
As per the Python `select.poll` documentation (https://docs.python.org/3/library/select.html#select.poll) and eventlet select removal advice (eventlet/eventlet#608 (comment)), it is preferable to use an implementation based on the availability of the `poll()` method that trying to check if the platform is `win32`.

Fixes docker#3131

Signed-off-by: Mathieu Virbel <mat@meltingrocks.com>
felixfontein added a commit to ansible-collections/community.docker that referenced this pull request May 20, 2023
* socket: fix for errors on pipe close in Windows (docker/docker-py#3099)

Need to return data, not size. By returning an empty
string, EOF will be detected properly since `len()`
will be `0`.

Fixes docker/docker-py#3098.

Cherry-picked from docker/docker-py@f846232

Co-authored-by: Milas Bowman <milas.bowman@docker.com>

* socket: use poll() instead of select() except on Windows (docker/docker-py#2865)

Fixes docker/docker-py#2278, which was originally addressed in docker/docker-py#2279, but was not
properly merged. Additionally it did not address the problem
of poll not existing on Windows. This patch falls back on the
more limited select method if host system is Windows.

Cherry-picked from docker/docker-py@a02ba74

Co-authored-by: Tyler Westland <tylerofthewest@gmail.com>

* api: respect timeouts on Windows named pipes (docker/docker-py#3112)

Cherry-picked from docker/docker-py@9cadad0

Co-authored-by: Imogen <59090860+ImogenBits@users.noreply.github.com>

* Add URL to changelog.

* api: avoid socket timeouts when executing commands (docker/docker-py#3125)

Only listen to read events when polling a socket in order
to avoid incorrectly trying to read from a socket that is
not actually ready.

Cherry-picked from docker/docker-py@c5e582c

Co-authored-by: Loïc Leyendecker <loic.leyendecker@gmail.com>

---------

Co-authored-by: Milas Bowman <milas.bowman@docker.com>
Co-authored-by: Tyler Westland <tylerofthewest@gmail.com>
Co-authored-by: Imogen <59090860+ImogenBits@users.noreply.github.com>
Co-authored-by: Loïc Leyendecker <loic.leyendecker@gmail.com>
milas pushed a commit that referenced this pull request Jun 1, 2023
Check if poll attribute exists on select module instead of win32 platform check

The implementation done in #2865 is breaking usage of docker-py library within eventlet.
As per the Python `select.poll` documentation (https://docs.python.org/3/library/select.html#select.poll) and eventlet select removal advice (eventlet/eventlet#608 (comment)), it is preferable to use an implementation based on the availability of the `poll()` method that trying to check if the platform is `win32`.

Fixes #3131

Signed-off-by: Mathieu Virbel <mat@meltingrocks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

exec_run: filedescriptor out of range in select() (python3)
3 participants