makefile() read() blocking - intended behavior?

[Colin Marc]

The code here: https://gist.github.com/2407326 seems to highlight inconsistent behavior in python2.7 with and without gevent.

In summary:

-call makefile() on a client socket

-settimeout(1.0) on the socket

-call read(100) on the fd

But:

-In gevent, read() blocks until the client disconnects (I was testing with telnet), and reports that the read data was '' (an empty string)

-Without gevent, read() raises a socket.timeout after a second

Also, it seems like some tests were built around this behavior. For example, test__server.TestDefaultSpawn.test_error_in_spawn. That test raises an error, then asserts that an empty string was read from the socket. 

I'm happy to create an issue and/or pull request, if this isn't intended behavior, but the tests made me pause. This came up in the gevent-py3k work I'm doing, because that test was failing even though we seemed to be mimicking the system makefile() behavior.

Thanks!

Colin Marc

[Matthias Urlichs]

On Tue, 2012-04-17 at 09:56 -0700, Colin Marc wrote:
> -In gevent, read() blocks until the client disconnects (I was testing
> with telnet), and reports that the read data was '' (an empty string)
> -Without gevent, read() raises a socket.timeout after a second

Looks like a bug to me. Ignoring an explicitly-set timeout can't be OK.

> Also, it seems like some tests were built around this behavior.

This clearly shows the requirement, for any kind of test-driven
development, to write the tests *first*, instead of merely using them to
document (un)intended behavior.

--
-- Matthias Urlichs

[Denis Bilenko]

Hi Colin,

On Tue, Apr 17, 2012 at 8:56 PM, Colin Marc <coli...@gmail.com> wrote:
> -In gevent, read() blocks until the client disconnects (I was testing with
> telnet), and reports that the read data was '' (an empty string)
> -Without gevent, read() raises a socket.timeout after a second

It's not intended. Sometime ago we had dup() inheriting timeout.
However, that was wrong and we fixed that. The makefile() used dup()
and thus stopped inheriting timeout as well. No one noticed until now.

Thanks for the report!

Fixed here: https://bitbucket.org/denis/gevent/changeset/65968cf1baa9

BTW, the reason nobody noticed it before is probably the fact that
fileobject is quite useless in the presence of socket timeouts as it
loses some of the buffered data in case of a timeout firing. The only
sensible response to timeout firing therefore is to close the
connection.

> Also, it seems like some tests were built around this behavior. For example,
> test__server.TestDefaultSpawn.test_error_in_spawn. That test raises an
> error, then asserts that an empty string was read from the socket.

I'm not sure how this test is related to the issue at hand. Could you elaborate?

Cheers,
Denis.

[Colin Marc]

That is one example of a test that times out with the correct behavior, which fails the test. I can post some code later, and perhaps actually make myself useful and fix the tests. =) For now, check out this function in test__server.py:

    def assertAcceptedConnectionError(self):

        sock, conn = self.makefile()

        result = conn.read(100)

        assert not result, repr(result)

That code is used in many tests where the server is 'killed' in some way - for example, the test I posted sets StreamServer._spawn to a lambda that throws an exception. The test wants to assert that it got an empty string, which is the old behavior, but it times out on read(100).

[Denis Bilenko]

On Wed, Apr 18, 2012 at 4:27 PM, Colin Marc <coli...@gmail.com> wrote:
> That is one example of a test that times out with the correct behavior,
> which fails the test. I can post some code later, and perhaps actually make
> myself useful and fix the tests. =) For now, check out this function in
> test__server.py:
>
>     def assertAcceptedConnectionError(self):
>         sock, conn = self.makefile()
>         result = conn.read(100)
>         assert not result, repr(result)
>
> That code is used in many tests where the server is 'killed' in some way -
> for example, the test I posted sets StreamServer._spawn to a lambda that
> throws an exception. The test wants to assert that it got an empty string,
> which is the old behavior, but it times out on read(100).

I think read() returns '' in this case because the server closed the
connection. You can verify that by disabling socket timeouts
altogether. It'll still return ''.

[Colin Marc]

Ah, you are correct. I was mixing up the client_socket on the server and the actual client socket. Thanks!