Actually the class asyncore.dispatcher_with_send do not handle properly disconnection. When the endpoint shutdown his sending part of the socket, but keep the socket open in reading, the current implementation of dispatcher_with_send will close the socket without sending pending data.

Adding this method to the class fix the problem:

    def handle_close(self):
        if not self.writable():
            dispatcher.close()

Note also that this class try to initiate a send even if the socket is maybe not ready for writing:

Here's a simple fix:
    def send(self, data):
        if self.debug:
            self.log_info('sending %s' % repr(data))
        self.out_buffer = self.out_buffer + data
-       self.initiate_send()

Last but not last, the buffer size of each socket send are way to small (512, a third of an usual TCP frame). Here's the code with a bumped value:

    def initiate_send(self):
        num_sent = 0
-       num_sent = dispatcher.send(self, self.out_buffer[:512])
+       num_sent = dispatcher.send(self, self.out_buffer[:8192])
        self.out_buffer = self.out_buffer[num_sent:]

Thanks for reading,

Hello,

> Actually the class asyncore.dispatcher_with_send do not handle properly
> disconnection. When the endpoint shutdown his sending part of the socket,
> but keep the socket open in reading, the current implementation of
> dispatcher_with_send will close the socket without sending pending data.

Yes, that's a common problem with "naive" networking code.

> Note also that this class try to initiate a send even if the socket is maybe
> not ready for writing:
>
> Here's a simple fix:
>     def send(self, data):
>         if self.debug:
>             self.log_info('sending %s' % repr(data))
>         self.out_buffer = self.out_buffer + data
> -       self.initiate_send()
>

Hum, I'm not sure about this.
dispatcher is just a thin wrapper around the underlying socket, so the
semantic of `send` should be the same as `socket.send`, i.e. just call
the send(2) syscall. I think it's the application's reponsibility to
check that the socket is indeed writable, and to cope with potential
failures (e.g. EAGAIN or ENOTCONN).

> Last but not last, the buffer size of each socket send are way to small
> (512, a third of an usual TCP frame). Here's the code with a bumped value:

Indeed, 1/3 of the ethernet MTU is quite small.

Would you like to submit a patch?

I am currently busy, but i will try to allocate some time to propose a
path soon.

Cheers,

2011/7/24 Charles-François Natali <report@bugs.python.org>:
>
> Charles-François Natali <neologix@free.fr> added the comment:
>
> Hello,
>
>> Actually the class asyncore.dispatcher_with_send do not handle properly
>> disconnection. When the endpoint shutdown his sending part of the socket,
>> but keep the socket open in reading, the current implementation of
>> dispatcher_with_send will close the socket without sending pending data.
>
> Yes, that's a common problem with "naive" networking code.
>
>> Note also that this class try to initiate a send even if the socket is maybe
>> not ready for writing:
>>
>> Here's a simple fix:
>>     def send(self, data):
>>         if self.debug:
>>             self.log_info('sending %s' % repr(data))
>>         self.out_buffer = self.out_buffer + data
>> -       self.initiate_send()
>>
>
> Hum, I'm not sure about this.
> dispatcher is just a thin wrapper around the underlying socket, so the
> semantic of `send` should be the same as `socket.send`, i.e. just call
> the send(2) syscall. I think it's the application's reponsibility to
> check that the socket is indeed writable, and to cope with potential
> failures (e.g. EAGAIN or ENOTCONN).
>
>> Last but not last, the buffer size of each socket send are way to small
>> (512, a third of an usual TCP frame). Here's the code with a bumped value:
>
> Indeed, 1/3 of the ethernet MTU is quite small.
>
> Would you like to submit a patch?
>
> ----------
> nosy: +neologix
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue12498>
> _______________________________________
>

> Actually the class asyncore.dispatcher_with_send do not handle
> properly disconnection. When the endpoint shutdown his sending part
> of the socket, but keep the socket open in reading, the current
> implementation of dispatcher_with_send will close the socket without
> sending pending data.
>
> Adding this method to the class fix the problem:
>
>     def handle_close(self):
>         if not self.writable():
>             dispatcher.close()


This does not seem to work. The attached Python 3.2 script
'asyncore_12498.py' goes into an infinite loop of calls to
handle_read/handle_close when handle_close is defined as above. In
this script the Writer sends a 4096 bytes message when connected, the
Reader reads only 64 bytes and closes the connection afterwards. Then
follows the sequence:

    select() returns a read event handled by handle_read()
    handle_read() calls recv()
    socket.recv() returns 0 to indicate a closed connection
    recv() calls handle_close

This sequence is repeated forever in asyncore.loop() since out_buffer
is never empty.

Note that after the first 'handle_close' has been printed, there are
no 'handle_write' printed, which indicates that the operating system
(here linux) states that the socket is not writable.

> This does not seem to work.

Yes, the proposed fix is buggy: the solution is to drain the output
buffer, guarding against recursive calls between send() and
handle_close() (patch attached).

> Note that after the first 'handle_close' has been printed, there are
> no 'handle_write' printed, which indicates that the operating system
> (here linux) states that the socket is not writable.

That's because you're closing the socket, whereas the original report
is dealing with half-shutdown connections.
Try this:
"""
 class Reader(asyncore.dispatcher):
     def __init__(self, sock):
         asyncore.dispatcher.__init__(self, sock)
+        self.shutdown = False

     def writable(self):
         return False

     def handle_read(self):
         self.recv(64)
-        self.close()
+        if not self.shutdown:
+            self.shutdown = True
+            self.socket.shutdown(socket.SHUT_WR)

 class Writer(asyncore.dispatcher_with_send):
     def __init__(self, address):
"""

This will behave as expected.
Calling socket.shutdown(socket.SHUT_WR) means that you won't send any
more data, but that you're still ready to receive: it results in a FIN
being sent to the remote endpoint, which will receive EOF on recv().
But subsequent send() from the remote endpoint will still succeed.
Whereas with socket.close(), you not only shut down your sending part,
but you also signify that you don't want to receive data any more: a
FIN will be sent immediately to the remote endpoint, and on subsequent
send() from the remote endpoint, a RST will be returned, which will
result in EPIPE (or, in there was data in the receive socket buffer
when the local endpoint was closed, a RST will be sent directly,
resulting in ECONNRESET).

I still need to write a test.

Thanks for the explanations.
I confirm that the patch fixes 'asyncore_12498.py' with your changes
applied to this script.

Note that the patch may break applications that have given different
semantics to 'closing' ('closing' being such a common name for a
network application) after they noticed that this attribute is never
used by asyncore nor by asynchat.

It seems that the same fix could also be applied to asynchat.

> Note that the patch may break applications that have given different
> semantics to 'closing' ('closing' being such a common name for a
> network application) after they noticed that this attribute is never
> used by asyncore nor by asynchat.

I was thinking about the discussion in issue 1641, the second part of
the discussion starting with msg 84972.

The attached patch uses another name and drains the output buffer only
on a close event, not on error conditions.

I will do the patch for asynchat and do both test cases, unless you
beat me to it.

> The attached patch uses another name

In that case, you should use a name starting with an underscore
("private" namespace).

> and drains the output buffer only on a close event, not on error conditions.

Hmmm...
I don't think it's a good idea.
For example, look at this in send():
"""
        except socket.error as why:
            # winsock sometimes throws ENOTCONN
            if why.args[0] in _DISCONNECTED:
                self.handle_close()
                return b''
"""

So it looks like on Windows, ENOTCONN can be returned instead of EOF:
the pending_close_evt flag wouldn't be set, and we wouldn't drain the
output buffer.
Also, some OSes can return POLLHUP without POLLIN when the remote end
shut down the connection:
http://blogs.oracle.com/vlad/entry/poll_and_pollhup_in_solaris
readwrite() would call handle_close() without setting the flag, and
the output buffer would also be lost (Note that the current code is
probably broken: when POLLHUP is received, this likely means that the
remote end has shutdown the connection, but there might still be some
data in the input socket buffer. I'll try to dig a little...).
So I'd rather stay on the safe side, and always try to drain the
output buffer in handle_close (worth case, we'll receive a
EPIPE/ECONNRESET, initiate_send will return 0 and we'll break out of
the loop).

> I will do the patch for asynchat and do both test cases, unless you
> beat me to it.

Go ahead :-)

While writing the test case, I found out that the test case does not
fail before the patch. It seems that draining the output buffer
already works:

The attached script 'asyncore_shutdown.py' drains the output buffer
when run without the patch, with Python 3.2, and prints 'Done.'. The
dispatcher_with_send handle_close() method is never called.

The attached 'asyncore_shutdown.log' file is the output of the tcpdump
of the connection. It shows that packets are sent after the first FIN.
This is on linux.

> While writing the test case, I found out that the test case does not
> fail before the patch. It seems that draining the output buffer
> already works:
>
> The attached script 'asyncore_shutdown.py' drains the output buffer
> when run without the patch, with Python 3.2, and prints 'Done.'. The
> dispatcher_with_send handle_close() method is never called.

That's because you didn't define a handle_read() method: since you
never read from the socket, you don't detect EOF, and never call
handle_close():

Try with this:
"""
             print('Done.')
             self.close()

+    def handle_read(self):
+        self.recv(MSG_SIZE)
+
     def handle_close(self):
         print('calling handle_close')
         asyncore.dispatcher_with_send.handle_close(self)
"""

And it'll fail ;-)

> That's because you didn't define a handle_read() method

Ooops...

Attached is a patch for dispatcher_with_send and asynchat with a test
case for each, that fail when the fix is not applied.

I've done a review of your patch (looks like Rietveld doesn't send emails).

Review done after Charles-François review.

Attached is a new patch following the code review.

After rewriting the asyncore test to check that the data has been
successfully received by the client, the test fails when
using poll() and a data size of 4096 bytes. The reason is that when
TestHandler closes the connection after writing the output buffer, the
client receives a POLLHUP which prevents it to receive the data since
POLLHUP is triggering a call to handle_close.

POSIX states:
"""
POLLHUP
    A device has been disconnected, or a pipe or FIFO has been closed
    by the last process that had it open for writing. Once set, the
    hangup state of a FIFO shall persist until some process opens the
    FIFO for writing or until all read-only file descriptors for the
    FIFO are closed. This event and POLLOUT are mutually-exclusive; a
    stream can never be writable if a hangup has occurred. However,
    this event and POLLIN, POLLRDNORM, POLLRDBAND, or POLLPRI are not
    mutually-exclusive. This flag is only valid in the revents
    bitmask; it shall be ignored in the events member.
"""

The attached patch changes the handling of POLLHUP to fix this: it
calls a new method named handle_hangup_evt() on a POLLHUP event, and
uses a new _hangup attribute to have writable() return False after a
POLLHUP. Note that we do get a close event (on linux) when all data
has been received, allowing the client to close the socket.

Please review this new patch.

> The reason is that when TestHandler closes the connection after 
> writing the output buffer, the client receives a POLLHUP which 
> prevents it to receive the data since POLLHUP is triggering a call to 
> handle_close.

Yes, that's the problem it noticed in http://bugs.python.org/issue12498#msg146645 :
"""
(Note that the current code is probably broken: when POLLHUP is
received, this likely means that the remote end has shutdown the
connection, but there might still be some data in the input socket
buffer. I'll try to dig a little...).
"""

I think the best would be to not handle POLLHUP events while POLLIN is set, so that the handlers can have a chance to drain the input socket buffer.
But it's a separate issue, could you create a new one?
You may also add a patch (in the same issue) to remove the useless setting of input flags to poll():
"""
        if flags:
            # Only check for exceptions if object was either readable
            # or writable.
            flags |= select.POLLERR | select.POLLHUP | select.POLLNVAL
            pollster.register(fd, flags)
"""

POLLERR, POLLHUP and POLLNVAL don't make sense when passed as input to poll() (they're set automatically in the output revents).

Follow my comments about half_duplex_close.diff (current latest patch).

+    def handle_close(self):
+        if not self._closing:
+            self._closing = True
+            # try to drain the output buffer
+            while self.writable() and self.initiate_send() > 0:
+                pass
+        self.close()

*Any* while loop should be avoided in the dispatcher. 
The risk is you keep the poller busy for more than a single loop or, at worst, forever.

Also, you expect initiate_send() to return a meaningful value which breaks compatibility with existing asynchat code overriding it.

> I think the best would be to not handle POLLHUP events while POLLIN
> is set, so that the handlers can have a chance to drain the input
> socket buffer.

Ok.


> But it's a separate issue, could you create a new one?

The test case fails if the data has not been fully received by the
client. Do you mean that this new issue must be fixed first. Why a new
issue, after all this is the handling of a half-duplex disconnection
on the remote side ?

Follow my comments about half_duplex_close.diff (current latest patch).

+    def handle_close(self):
+        if not self._closing:
+            self._closing = True
+            # try to drain the output buffer
+            while self.writable() and self.initiate_send() > 0:
+                pass
+        self.close()

> *Any* while loop should be avoided in the dispatcher. 
> The risk is you keep the poller busy for more than a single loop or,
> at worst, forever.

Right. I will try to change that.


> Also, you expect initiate_send() to return a meaningful value which
> breaks compatibility with existing asynchat code overriding it.

The patch changes also initiate_send() accordingly in asynchat.

> this is the handling of a half-duplex disconnection on the remote
> side ?

Actually this is not the handling of a half-duplex disconnection on the
remote side, but we need a half-duplex disconnection to test it.

Attached yet another patch.
This patch does not use a while loop in handle_close() and handles
POLLHUP as suggested by Charles-François. No changes have been made to
both tests (test_half_duplex_close).

I think this thread is becoming a little messy and since asyncore/asynchat are in a situation where even the slightest change can break existent code I recommend to be really careful.

I see 3 different issues here:

1 - dispatcher_with_send closing the socket without sending pending data (this was the initial issue)
2 - dispatcher_with_send default buffer is too small (512 bytes)
3 - add support for correct POLLHUP/socket.shutdown() handling (msg146946)

All of them should be treated and discussed separately in their own ticket to figure out:

- what's wrong
- whether it's a bugfix or a new feature (POLLHUP handling appears to be so)
- for what python version(s) the patch should be applied

As a final note we should consider mandatory for any patch not to alter the existent API.
initiate_send() method suddenly returning a meaningful value might be the case and as such it should be weighed up first.

Other asyncore issues have been closed as "won't fix" as it's been deprecated in favour of asyncio.  I assume the same applies here.

"Actually the class asyncore.dispatcher_with_send do not handle properly disconnection. When the endpoint shutdown his sending part of the socket, but keep the socket open in reading, the current implementation of dispatcher_with_send will close the socket without sending pending data."

It looks like asyncore doesn't handle this use case.

To me, it doesn't look like a minor issue, but more a design issue. Fixing it requires to change the design of asyncore.

The asyncio module has a better design. It has a write_eof() method to close the write end without touching the read end. For example, for sockets, write_eof() calls sock.shutdown(socket.SHUT_WR). After write_eof(), the read continues in background as any other task. For the read end, the protocol has a eof_received() method to decide if the socket should close, or if it should be kept open for writing (but only for writing).

Giampaolo wrote:
> I think this thread is becoming a little messy and since asyncore/asynchat are in a situation where even the slightest change can break existent code I recommend to be really careful.

Moreover, the asyncore module has been deprecated in favor of the asyncio module.

I close this issue for all these reasons.

Sorry Xavier for your patches, but it's time to focus our efforts on a single module and asyncio has a much better design to handle such use cases.

No worries, I am glad to see asyncore going away. It was indeed badly
designed in the first place.

--
François-Xavier Bourlet

On Fri, Jun 27, 2014 at 2:28 PM, STINNER Victor <report@bugs.python.org> wrote:
>
> STINNER Victor added the comment:
>
> "Actually the class asyncore.dispatcher_with_send do not handle properly disconnection. When the endpoint shutdown his sending part of the socket, but keep the socket open in reading, the current implementation of dispatcher_with_send will close the socket without sending pending data."
>
> It looks like asyncore doesn't handle this use case.
>
> To me, it doesn't look like a minor issue, but more a design issue. Fixing it requires to change the design of asyncore.
>
> The asyncio module has a better design. It has a write_eof() method to close the write end without touching the read end. For example, for sockets, write_eof() calls sock.shutdown(socket.SHUT_WR). After write_eof(), the read continues in background as any other task. For the read end, the protocol has a eof_received() method to decide if the socket should close, or if it should be kept open for writing (but only for writing).
>
> Giampaolo wrote:
>> I think this thread is becoming a little messy and since asyncore/asynchat are in a situation where even the slightest change can break existent code I recommend to be really careful.
>
> Moreover, the asyncore module has been deprecated in favor of the asyncio module.
>
> I close this issue for all these reasons.
>
> Sorry Xavier for your patches, but it's time to focus our efforts on a single module and asyncio has a much better design to handle such use cases.
>
> ----------
> nosy: +haypo
> resolution:  -> wont fix
> status: open -> closed
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue12498>
> _______________________________________

> Sorry Xavier for your patches, but it's time to focus our efforts on a single module and asyncio has a much better design to handle such use cases.

No problem. Thanks for taking your time to review patches made on this old module.