Issue12498
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2011-07-05 01:55 by François-Xavier.Bourlet, last changed 2022-04-11 14:57 by admin. This issue is now closed.
Files | ||||
---|---|---|---|---|
File name | Uploaded | Description | Edit | |
asyncore_12498.py | xdegaye, 2011-10-29 19:32 | |||
asyncore_drain.diff | neologix, 2011-10-30 11:16 | review | ||
asyncore_drain_2.diff | xdegaye, 2011-10-30 15:59 | review | ||
asyncore_shutdown.py | xdegaye, 2011-10-30 17:49 | |||
asyncore_shutdown.log | xdegaye, 2011-10-30 18:02 | |||
asyncore_drain_3.diff | xdegaye, 2011-10-30 21:48 | review | ||
half_duplex_close.diff | xdegaye, 2011-11-03 16:43 | review | ||
half_duplex_close_2.diff | xdegaye, 2011-11-04 17:12 | review |
Messages (25) | |||
---|---|---|---|
msg139821 - (view) | Author: François-Xavier Bourlet (François-Xavier.Bourlet) | Date: 2011-07-05 01:55 | |
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, |
|||
msg141057 - (view) | Author: Charles-François Natali (neologix) * | Date: 2011-07-24 20:51 | |
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? |
|||
msg141619 - (view) | Author: François-Xavier Bourlet (François-Xavier.Bourlet) | Date: 2011-08-04 00:43 | |
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> > _______________________________________ > |
|||
msg146618 - (view) | Author: Xavier de Gaye (xdegaye) * | Date: 2011-10-29 19:32 | |
> 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. |
|||
msg146630 - (view) | Author: Charles-François Natali (neologix) * | Date: 2011-10-30 11:16 | |
> 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. |
|||
msg146635 - (view) | Author: Xavier de Gaye (xdegaye) * | Date: 2011-10-30 12:36 | |
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. |
|||
msg146642 - (view) | Author: Xavier de Gaye (xdegaye) * | Date: 2011-10-30 15:59 | |
> 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. |
|||
msg146645 - (view) | Author: Charles-François Natali (neologix) * | Date: 2011-10-30 17:09 | |
> 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 :-) |
|||
msg146649 - (view) | Author: Xavier de Gaye (xdegaye) * | Date: 2011-10-30 18:02 | |
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. |
|||
msg146653 - (view) | Author: Charles-François Natali (neologix) * | Date: 2011-10-30 18:43 | |
> 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 ;-) |
|||
msg146664 - (view) | Author: Xavier de Gaye (xdegaye) * | Date: 2011-10-30 21:48 | |
> 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. |
|||
msg146857 - (view) | Author: Charles-François Natali (neologix) * | Date: 2011-11-02 17:45 | |
I've done a review of your patch (looks like Rietveld doesn't send emails). |
|||
msg146874 - (view) | Author: Xavier de Gaye (xdegaye) * | Date: 2011-11-02 20:10 | |
Review done after Charles-François review. |
|||
msg146946 - (view) | Author: Xavier de Gaye (xdegaye) * | Date: 2011-11-03 16:43 | |
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. |
|||
msg146956 - (view) | Author: Charles-François Natali (neologix) * | Date: 2011-11-03 17:56 | |
> 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). |
|||
msg146958 - (view) | Author: Giampaolo Rodola' (giampaolo.rodola) * | Date: 2011-11-03 18:19 | |
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. |
|||
msg146970 - (view) | Author: Xavier de Gaye (xdegaye) * | Date: 2011-11-03 20:08 | |
> 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 ? |
|||
msg146971 - (view) | Author: Xavier de Gaye (xdegaye) * | Date: 2011-11-03 20:12 | |
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. |
|||
msg146974 - (view) | Author: Xavier de Gaye (xdegaye) * | Date: 2011-11-03 20:27 | |
> 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. |
|||
msg147016 - (view) | Author: Xavier de Gaye (xdegaye) * | Date: 2011-11-04 17:12 | |
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). |
|||
msg147042 - (view) | Author: Giampaolo Rodola' (giampaolo.rodola) * | Date: 2011-11-04 21:27 | |
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. |
|||
msg221210 - (view) | Author: Mark Lawrence (BreamoreBoy) * | Date: 2014-06-21 23:21 | |
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. |
|||
msg221739 - (view) | Author: STINNER Victor (vstinner) * | Date: 2014-06-27 21:28 | |
"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. |
|||
msg221741 - (view) | Author: François-Xavier Bourlet (François-Xavier.Bourlet) | Date: 2014-06-27 21:30 | |
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> > _______________________________________ |
|||
msg222274 - (view) | Author: Xavier de Gaye (xdegaye) * | Date: 2014-07-04 10:33 | |
> 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. |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-11 14:57:19 | admin | set | github: 56707 |
2014-07-04 10:33:25 | xdegaye | set | messages: + msg222274 |
2014-06-27 21:30:57 | François-Xavier.Bourlet | set | messages: + msg221741 |
2014-06-27 21:28:11 | vstinner | set | status: open -> closed nosy: + vstinner messages: + msg221739 resolution: wont fix |
2014-06-21 23:21:01 | BreamoreBoy | set | nosy:
+ BreamoreBoy messages: + msg221210 |
2011-11-04 21:27:10 | giampaolo.rodola | set | messages: + msg147042 |
2011-11-04 17:12:53 | xdegaye | set | files:
+ half_duplex_close_2.diff messages: + msg147016 |
2011-11-03 20:27:42 | xdegaye | set | messages: + msg146974 |
2011-11-03 20:12:38 | xdegaye | set | messages: + msg146971 |
2011-11-03 20:08:44 | xdegaye | set | messages: + msg146970 |
2011-11-03 18:19:12 | giampaolo.rodola | set | messages: + msg146958 |
2011-11-03 17:59:46 | pitrou | set | nosy:
+ josiahcarlson, giampaolo.rodola, stutzbach |
2011-11-03 17:56:12 | neologix | set | messages: + msg146956 |
2011-11-03 16:43:44 | xdegaye | set | files:
+ half_duplex_close.diff messages: + msg146946 |
2011-11-02 20:10:57 | xdegaye | set | messages: + msg146874 |
2011-11-02 17:45:34 | neologix | set | messages:
+ msg146857 stage: needs patch -> patch review |
2011-10-30 21:48:29 | xdegaye | set | files:
+ asyncore_drain_3.diff messages: + msg146664 |
2011-10-30 18:43:24 | neologix | set | messages: + msg146653 |
2011-10-30 18:02:28 | xdegaye | set | files:
+ asyncore_shutdown.log messages: + msg146649 |
2011-10-30 17:49:53 | xdegaye | set | files: + asyncore_shutdown.py |
2011-10-30 17:09:41 | neologix | set | messages: + msg146645 |
2011-10-30 15:59:53 | xdegaye | set | files:
+ asyncore_drain_2.diff messages: + msg146642 |
2011-10-30 12:36:40 | xdegaye | set | messages: + msg146635 |
2011-10-30 11:16:23 | neologix | set | files:
+ asyncore_drain.diff keywords: + patch messages: + msg146630 |
2011-10-29 19:32:12 | xdegaye | set | files:
+ asyncore_12498.py nosy: + xdegaye messages: + msg146618 |
2011-08-04 00:43:52 | François-Xavier.Bourlet | set | messages: + msg141619 |
2011-07-24 20:52:01 | neologix | set | stage: needs patch type: behavior components: + Library (Lib), - IO versions: + Python 3.2, Python 3.3 |
2011-07-24 20:51:17 | neologix | set | nosy:
+ neologix messages: + msg141057 |
2011-07-05 01:55:29 | François-Xavier.Bourlet | create |