Skip to content

Conversation

kevinfrommelt
Copy link
Contributor

Problem

When streaming responses, socket timeouts are set to None to avoid read timeouts. Normally this is fine, but causes issues when using gevent. When using gevent, the socket class will be patched to be non-blocking (ie timeout == 0.0). If the timeout is set to None the socket becomes blocking again causing all greenlets to be blocked until the stream has finished.

Solution

The proposed fix is to only set the socket timeout if it isn't already disabled.

Reproduction Steps

test.py

from gevent import monkey
monkey.patch_all()

from datetime import datetime
import sys
import docker
import gevent

BUSYBOX = 'busybox:buildroot-2014.02'

def logs():
    for log in client.logs(container, stdout=True, stderr=True, stream=True):
        sys.stdout.write('[{}] log {}\n'.format(datetime.now().time(), log.strip()))
        sys.stdout.flush()

def ping():
    for i in xrange(0, 6):
        sys.stdout.write('[{}] ping {}\n'.format(datetime.now().time(), i))
        sys.stdout.flush()
        gevent.sleep(1)

if __name__ == '__main__':
    tls = docker.tls.TLSConfig(
        client_cert=(
            '/path/to/cert.pem',
            '/path/to/key.pem'
        ),
        verify='/path/to/ca.pem'
    )
    client = docker.Client(base_url='<docker_host>', tls=tls)

    client.pull(BUSYBOX)

    container = client.create_container(
        BUSYBOX,
        'sh -c "echo start; sleep 10; echo finish"',
        detach=True,
    )

    client.start(container)

    logs = gevent.spawn(logs)
    ping = gevent.spawn(ping)

    gevent.joinall([logs, ping], timeout=30)

Without these changes:

> python test.py
[10:02:20.749195] ping 0
[10:02:20.763980] log start
[10:02:30.751678] log finish
[10:02:30.772866] ping 1        <--- Gets blocked until logs have finished streaming (ie 10 secs)
[10:02:31.777957] ping 2
[10:02:32.781986] ping 3
[10:02:33.786190] ping 4
[10:02:34.790423] ping 5

With changes:

> python test.py
[10:02:48.505751] ping 0
[10:02:48.521915] log start
[10:02:49.510465] ping 1        <--- Allowed to run while streaming logs
[10:02:50.513776] ping 2
[10:02:51.517609] ping 3
[10:02:52.521996] ping 4
[10:02:53.526963] ping 5
[10:02:58.507385] log finish

@dnephin
Copy link
Contributor

dnephin commented May 6, 2016

Code change looks good. How about a test to cover this case?

@kevinfrommelt
Copy link
Contributor Author

@dnephin I added a couple unit tests. I think integration tests are going to be a bit trickier, as I'm not really sure how to replicate it without importing gevent and doing some patching. Let me know what you think, I can try playing around with some integration tests if you think we need it.

Signed-off-by: Kevin Frommelt <kevin.frommelt@gmail.com>
@dnephin
Copy link
Contributor

dnephin commented May 10, 2016

LGTM

@shin- shin- added this to the 1.9.0 milestone May 10, 2016
@shin- shin- merged commit 8b41679 into docker:master May 10, 2016
@shin-
Copy link
Contributor

shin- commented May 10, 2016

Thanks!

@kevinfrommelt kevinfrommelt deleted the socket-timeout branch May 11, 2016 12:14
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.

4 participants