8000 Use poll() instead of select() by bieron · Pull Request #2279 · docker/docker-py · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Use poll() instead of select() #2279

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Use poll() instead of select() #2279

wants to merge 1 commit into from

Conversation

bieron
Copy link
@bieron bieron commented Mar 18, 2019

Fixes #2278
because poll does not have select's builtin limit of 1024 descriptors

This pr does not contain any updates to tests although it probably should. I'm also not 100% sure it is a proper fix, but it does solve the issue of #2278 when tested manually.

@GordonTheTurtle
Copy link

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "master" git@github.com:bieron/docker-py.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@shin-
Copy link
Contributor
shin- commented Mar 20, 2019

Unfortunately, select.poll doesn't exist on Windows, so this'll definitely need more work -- maybe by continuing to use select.select if we're running on Windows.
I'm also curious as to whether there's a non-manufactured case where more than 1024 sockets are open on a client machine at the same time? That seems like a lot.

@bieron
Copy link
Author
bieron commented Mar 20, 2019 •
edited
Loading

You're correct about lack of select.poll. It's not only windows, but also using eventlet (e.g. in celery) magically patches your select module, removing poll from it.
As for is this a real life problem I would say yes - remember that the limit is for fds, not just sockets. We actually encountered this problem when a process that had opened over a thousand files attempted Container.exec_run

@ulyssessouza
Copy link
Contributor

This PR is not passing due to an already solved problem.
Please rebase your PR with master

@bieron
Use poll() instead of select()
2953fb6 
Fixes docker#2278
because poll does not have select's builtin limit of 1024 descriptors

Signed-off-by: Jan Bieron <jbieron+github@gmail.com>
@pdreiter pdreiter mentioned this pull request Jul 1, 2019
Merged
I-question-this added a commit to I-question-this/docker-py that referenced this pull request Jul 22, 2021
@I-question-this
Use poll() instead of select(), unless Windows.
69742c4 
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>
8000 @I-question-this I-question-this mentioned this pull request Jul 22, 2021
Merged
@yugokato
Copy link

Hi, just wanted to check in with this as I encounter the same issue. What is the status on this?

/usr/local/lib/python3.9/site-packages/docker/models/containers.py:198: in exec_run
    exec_output = self.client.api.exec_start(
/usr/local/lib/python3.9/site-packages/docker/utils/decorators.py:19: in wrapped
    return f(self, resource_id, *args, **kwargs)
/usr/local/lib/python3.9/site-packages/docker/api/exec_api.py:167: in exec_start
    return self._read_from_socket(res, stream, tty=tty, demux=demux)
/usr/local/lib/python3.9/site-packages/docker/api/client.py:424: in _read_from_socket
    return consume_socket_output(gen, demux=demux)
/usr/local/lib/python3.9/site-packages/docker/utils/socket.py:135: in consume_socket_output
    return bytes().join(frames)
/usr/local/lib/python3.9/site-packages/docker/api/client.py:418: in <genexpr>
    gen = (data for (_, data) in gen)
/usr/local/lib/python3.9/site-packages/docker/utils/socket.py:92: in frames_iter_no_tty
    (stream, n) = next_frame_header(socket)
/usr/local/lib/python3.9/site-packages/docker/utils/socket.py:64: in next_frame_header
    data = read_exactly(socket, 8)
/usr/local/lib/python3.9/site-packages/docker/utils/socket.py:49: in read_exactly
    next_data = read(socket, n - len(data))
/usr/local/lib/python3.9/site-packages/docker/utils/socket.py:29: in read
    select.select([socket], [], [])
E   ValueError: filedescriptor out of range in select()

milas pushed a commit that referenced this pull request Apr 21, 2023
@I-question-this
socket: use poll() instead of select() except on Windows (#2865)
a02ba74 
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>
felixfontein added a commit to felixfontein/community.docker that referenced this pull request May 10, 2023
@felixfontein @I-question-this
socket: use poll() instead of select() except on Windows (docker/dock…
5a4b4d0 
…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>
felixfontein added a commit to ansible-collections/community.docker that referenced this pull request May 20, 2023
@felixfontein @milas
@I-question-this @ImogenBits @loicleyendecker
vendored Docker SDK for Python code: update to latest version (#619)
1660bf4 
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aiordache aiordache Awaiting requested review from aiordache

@ulyssessouza ulyssessouza Awaiting requested review from ulyssessouza

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

exec_run: filedescriptor out of range in select() (python3)
5 participants
@bieron @GordonTheTurtle @shin- @ulyssessouza @yugokato
0