| msg146785 - (view) |
Author: Xavier de Gaye (xdegaye) *  |
Date: 2011-11-01 16:08 |
| When the remote end disconnects, handle_close is only called if recv is called (from handle_read). The default implementation of handle_read does not call recv. Not having the default implementation of handle_read call recv, has the following drawbacks: an implementation of a subclass of dispatcher that only sends data, a logger for example, may believe that it does not have to implement handle_read since it does not expect any data and since there is no hint in the code or in the documentation that it should test_handle_expt currently succeeds when it should fail since the current handling of out-of-band data is broken (see issue 13310), but if the default implementation of handle_read had called recv, then test_handle_expt would have failed, allowing to detect the problem The attached patch adds a call to recv in handle_read, updates the documentation and adds a test case. Note that when this patch is applied, test_handle_expt fails as expected, issue 13310 should be fixed first. |
|
|
| msg146933 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) *  |
Date: 2011-11-03 13:15 |
| > When the remote end disconnects, handle_close is only called if recv > is called (from handle_read). Actually this isn't true; handle_close() is also called in send(): http://hg.python.org/cpython/file/eb2991f7cdc8/Lib/asyncore.py#l364 I'd say your patch can be useful only in case the dispatcher subclass doesn't send() neither recv() any data, in which case the connection is supposed to remain open forever. On one hand this might be a good thing, on the other hand I'm not sure what the repercussions might be in the existing code base out there. Probably none, but... Perhaps you could provide more info about why you needed to do this in the first place. Did you encounter a specific use case requiring this patch in order to work? If so, please paste the code where you subclassed asyncore.dispatcher. |
|
|
| msg146948 - (view) |
Author: Xavier de Gaye (xdegaye) * |
Date: 2011-11-03 17:13 |
| > I'd say your patch can be useful only in case the dispatcher subclass > doesn't send() neither recv() any data, in which case the connection > is supposed to remain open forever. There are some cases where it is important to detect that the remote end is disconnected even if there is no data to send. Say a logger connected to a data collector that sends data every few minutes. The data collector dies, the logger may have to take actions on this event: connect to a backup collector, raise an alarm, whatever... It should not have to depend on the fact that data needs to be sent to learn of the disconnection. > Perhaps you could provide more info about why you needed to do this > in the first place. See also issue 12498 and the message 146653. When the remote end performs a half-duplex disconnection, you may send data without detecting the close event in send(), so you must rely on recv() to detect it. |
|
|
| msg146954 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2011-11-03 17:43 |
| > The attached patch adds a call to recv in handle_read, updates the > documentation and adds a test case. In this kind of situation, it is perfectly legitimate for the client to perform a half-duplex close (shutdown(SHUT_WR)), since it does not intend to send data (which is implied by the fact that the sever doesn't implement an handle_read() method). With the current code, the server will keep sending data until the remote end close()s its socket. With this patch, this would break: the server's handle_close() method will be called right away. > There are some cases where it is important to detect that the remote > end is disconnected even if there is no data to send. Indeed, but if you must detect in a timely maner that the remote end close()d its connection, you should provide an handle_read() method. I don't think that breaking perfectly valid code to help hypothetical sloppy applications is a good idea. Here's what the doc says: """ handle_close() | Implied by a read event with no data available [...] handle_close() |
Called when the socket is closed. """ Note that "Called when the socket is closed" is confusing: it should probably be rephrased as "Called when the remote endpoint has performed a shutdown", or something along those lines. |
|
| msg146966 - (view) |
Author: Xavier de Gaye (xdegaye) * |
Date: 2011-11-03 19:54 |
| > In this kind of situation, it is perfectly legitimate for the client > to perform a half-duplex close (shutdown(SHUT_WR)), since it does > not intend to send data (which is implied by the fact that the sever > doesn't implement an handle_read() method). > With the current code, the server will keep sending data until the > remote end close()s its socket. > With this patch, this would break: the server's handle_close() > method will be called right away. Since this patch may break existing valid code, I think it should be closed as invalid. |
|
|
| msg147022 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2011-11-04 18:18 |
| > Since this patch may break existing valid code, I think it should be > closed as invalid. Yes. Since the benefit is not clear and it may break existing code, it's probably wiser. |
|
|