When using asynchat.async_chat channel, and connection is refused, 
asyncore fail incorrectly.

First, instead of ECONNREFUSED, you get EPIPE, second, you also get a 
EBADF exception, and finally, the channel handle_close is called  twice.

The problem is the way asyncore detect a connect event. On the first 
read event, asyncore set the connected flag to true, and invoke the 
channel handle_connect method. Typically, the channel will try to push 
something to the remote endpoint at this point. Since "connected" is 
true, push() will try to send to the socket, and fail with EPIPE, 
because the socket is not connected.

The socket error in push will invoke the channel handle_error, which 
typically will call handle_close to close the socket. 
handle_connect_event will return without any error, because the error 
was handled inside push.

Now asyncore will invoke the channel handle_read method, which fails 
with bad file descriptor, since the channel closed the socket on the 
previous error.

How should this work correctly?

1. We want to get a connection refused error in this case
2. The failure should cause only single exception
3. The channel should be close once

I added these test to test_asyncore.py, and all of them fail with 
current code.

Turns out that single line change fix all the failing tests: set the 
"connected" state after the call to handle_connect, just like it used to 
be in 2.5.

With this change, when the channel try to push something to the remote 
endpoint, it will keep it in the fifo, since connected is false. This is 
also correct, since we will know that we are connected only after 
handle_read is called. In handle_read, we fail with ECONNREFUSED and 
close the channel.

The fix is tested currently only on Mac OS X 10.5. I have seen this 
issue also on Ubuntu 9.04.

The patch is tested with release26-maint and trunk.

Tested on Ubuntu Linux 9.04.

The tests will probably fail on Windows, since connection refused is
detected trough handle_expt_event, and not in hadnle_read_event. I hope
someone on Windows will fix this :-)

The first fix reverted to 2.5 behavior, when self.connected is false when 
handle_connect is called. This behavior is little stupid - why call 
handle_connect if the socket is not really connected?

This fix ensure that handle_connect is called only if the socket is 
connected - just like it is done in handle_write_event. Now it does not 
matter when you set the connected flag, before or after handle_connect.

Tested in trunk on Mac OS X.

This version fix also handle_expt_event, so connection refused error 
should be handled in the same way also on Windows.

The attached patch cleans up the remnants of the "handle_expt is for 
exceptions", which isn't the case, as well as makes the "connection 
refused" fix actually work on Windows.  Nirs, could you verify this on 
*nix?

I have a big problem with asyncore_fix_refused.patch  - it assumes that a 
dispatcher has a socket attribute, which can be used with t getsockopt(). 
This is true in the default dispatcher class implemented in asyncore, but 
wont work with file_dispatcher, or 3rd party dispatcher class.

The framework should let you drop in your dispatcher class, that will 
implement a minimal interface - handle_x_event, writable, readable etc.

What are the issues on Windows with asyncore-handle-connect-event-3.patch 
on Windows?

Firstly, it expects that handle_expt_event() is for handling exceptional 
conditions.  This is not the case.  handle_expt_event() is meant for 
handling "OOB" or "priority" data coming across a socket.  FTP and some 
other protocols use this.  I forgot to fix it earlier, which is why it's 
making it into this patch.

Secondly, I pulled the part that was inside handle_expt_event() that was 
being used to find the exception and pulls it out into _exception(), 
removing the previous behavior (wrt to the broken API), and replacing it 
with something that is cleaner and more correct (assuming sockets).

To respond to it being an issue that the object has a socket with a 
getsockopt(), I can fix it to handle the AttributeError case.

Would you be willing to try this out given my explanation as to why I 
changed your patch?

I'll check the patch this week.

The asyncore framework has low level events - handle_read_event, handle_write_event and 
handle_expt_event - these events are not used for reading, writing and OOB - they are just 
responsible to call the high level events.

The high level events - handle_connect, handle_accept, handle_read, handle_write, 
handle_close and handle_expt should be used only for specific events.

I don't see any problem in checking for errors in handle_expt_event, it works just like 
handle_read_event, that calls handle_connect.

This design allow you do replace the dispatcher with your own dispatcher class, 
implementing only the low level events.

Originally, handle_expt_event() was described as "handles OOB data or 
exceptions", but over-using handle_expt_event() as an error/close 
handler is a bad idea.  The function asyncore.readwrite() (called by 
asyncore.poll2()) does the right thing WRT handle_expt_event(), which it 
makes sense to apply to the standard select-based asyncore.poll().  
That's what this does (in addition to fixing the close case that you 
pointed out).

In terms of "only implementing low-level stuff", this is still the case.  
You still only need to implement handle_*(), not handle_*_event() .  But 
now, handle_expt_event() isn't written to do more than it should have 
been doing in the first place.

I've updated the patch to include semantics for actually handling OOB 
data, which I've verified by using a slightly modified pyftpdlib (remove 
the socket option calls to set socket.SO_OOBINLINE) and it's tests on 
both Windows and Ubuntu 8.04 (I also ran the full Python test suite on 
my Ubuntu install, and any failures were obviously not asyncore/asynchat 
related).

I tested asyncore_fix_refused-3.patch on Mac OS X 10.5 - all asyncore 
and asynchat tests pass.

There is one minor issue - _exception calls the non existing 
handle_close_event instead of handle_close.

However, looking again at the code I think that it is ugly and wrong - 
handle_expt_event should handle the low level expt event called from 
select, allowing third party dispatcher to override the behavior as 
needed.

Another issue - lately, a new event was added - handle_connect_event - 
this is wrong! there is no such low level event. handle_connect is a 
high level event, implied by first read or write on the connecting 
socket. This event will break 3rd party dispatcher that does not 
implement it, and is not documented.

I was wrong about handle_connect_event - it is called only from the 
dispatcher, so it will not break 3rd party dispatcher.

This is asyncore-fix-refused-3.patch with some fixes:

 1. Call handle_close instead of non exiting handle_close_event
 2. Remove unneeded handle_close_event in test classes
 3. Revert removal of handle_expt_event in test classes - not clear why it 
was removed in the previous patch.

Tested on Mac OS X 10.5.

handle_expt_event was removed in the test classes because it is no 
longer being used by any of the tests.  None of them send OOB data (also 
known as priority data), so handle_expt_event should never be called.  
When I have a chance to compare your patch to mine (Monday, likely), 
I'll comment then.

In terms of "handle_expt_event should handle the low level expt event 
called from select", what you don't seem understand is that and "expt 
event" is not an exception event, it's a "we got OOB data" event, and 
with the patches, it is called at *exactly* the time it should be: when 
there is OOB data.  It used to be mistakenly called in the select loop 
whenever any sort of non-normal condition happened on the socket, which 
was a serious design flaw, and lead to significant misunderstanding 
about the purpose of the method.

With the _exception() call as-is, it now behaves like the 
asyncore.poll2() function, which is the right thing.

handle_expt is documented to be called when there is OOB data. However,
handle_expt_event is not documented, and according the framework design
as I see it, it simply means "socket has exceptional condition" when
select returns. On unix, this means there is oob data, and on Windows,
it means "there is some error".

This works exactly the same for handle_read_event and handle_write_event
- they may be called on connection refused error. Checking for errors in
handle_expt_event is the right thing to do, and allow you to avoid the
ugly checks and double try..except in _exception.

If you want handle_foo_event to be called only on foo events, then they
will not have anything to do except calling handle_foo. This is actually
the case now in handle_expt_event. I don't see any advantage of this
design change.

What's the current status of this issue?
I think some of the problems raised at the time this was opened should have been fixed in meantime, like checking for errors in handle_expt_event and handle_connect_event (issue 2944).

The handle_close being called twice problem is still there though, as shown in issue 4690.

Not sure what other problems remain.

Has maintenance of asyncore effectively ceased owing to tulip/asyncio or are outstanding problems here, if any, still considered valid?

@Victor as you've been looking at other asyncore/chat issues can you look at this please?

The latest patch was written 5 years ago, it's probably a huge work to rebase it. Giampaolo wrote that the code changed a lot since this time (especially code handling errors).

asyncore-fix-refused-4.patch catchs EBADF. To me, it looks like a bug: if you get a EBADF error, we already made something wrong before.

The patch doesn't call obj.handle_expt_event() anymore in _exception() if obj is not a socket. It don't understand why. I don't understand neither why _exception() now raises a new socket error instead of calling obj.handle_expt_event().

All these changes are non trivial. I don't feel able to review the patch.

Sorry, I'm not interested to invest time on this deprecated module. It's time to move forward, to asyncio, and so I close this issue.