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.
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
asyncore.dispatcher_with_send, disconnection problem + miss-conception #56707
Comments
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:
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,
Yes, that's a common problem with "naive" networking code.
Hum, I'm not sure about this.
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 Cheers, 2011/7/24 Charles-François Natali report@bugs.python.org:
|
This does not seem to work. The attached Python 3.2 script
This sequence is repeated forever in asyncore.loop() since out_buffer Note that after the first 'handle_close' has been printed, there are |
Yes, the proposed fix is buggy: the solution is to drain the output
That's because you're closing the socket, whereas the original report 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. I still need to write a test. |
Thanks for the explanations. Note that the patch may break applications that have given different It seems that the same fix could also be applied to asynchat. |
I was thinking about the discussion in bpo-1641, the second part of The attached patch uses another name and drains the output buffer only I will do the patch for asynchat and do both test cases, unless you |
In that case, you should use a name starting with an underscore
Hmmm... So it looks like on Windows, ENOTCONN can be returned instead of EOF:
Go ahead :-) |
While writing the test case, I found out that the test case does not The attached script 'asyncore_shutdown.py' drains the output buffer The attached 'asyncore_shutdown.log' file is the output of the tcpdump |
That's because you didn't define a handle_read() method: since you Try with this: + def handle_read(self): And it'll fail ;-) |
Ooops... Attached is a patch for dispatcher_with_send and asynchat with a test |
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 POSIX states: The attached patch changes the handling of POLLHUP to fix this: it Please review this new patch. |
Yes, that's the problem it noticed in http://bugs.python.org/issue12498#msg146645 : 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. 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): *Any* while loop should be avoided in the dispatcher. Also, you expect initiate_send() to return a meaningful value which breaks compatibility with existing asynchat code overriding it. |
Ok.
The test case fails if the data has not been fully received by the |
Follow my comments about half_duplex_close.diff (current latest patch). + def handle_close(self):
Right. I will try to change that.
The patch changes also initiate_send() accordingly in asynchat. |
Actually this is not the handling of a half-duplex disconnection on the |
Attached yet another patch. |
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) All of them should be treated and discussed separately in their own ticket to figure out:
As a final note we should consider mandatory for any patch not to alter the existent API. |
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:
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 -- On Fri, Jun 27, 2014 at 2:28 PM, STINNER Victor <report@bugs.python.org> wrote:
|
No problem. Thanks for taking your time to review patches made on this old module. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: