[PATCH lib/libxtrans] If Socket is getting interrupted with signal EINTR, after re attempts we are closing socket connection, which is wrong.

[Arvind Umrao]

On 08/23/12 05:54 PM, Mark Kettenis wrote:
>> From: Arvind Umrao<arvind.umrao at oracle.com>
>> Date: Thu, 23 Aug 2012 16:07:39 +0530
>>
>> Code changes are integrated in Solaris and now I am trying to give back to community. If Socket is getting interrupted with signal EINTR, we should keep socket in progress state(TRANS_IN_PROGRESS) instead of trying again to connect(TRANS_TRY_CONNECT_AGAIN). When we close the socket connection on signal EINTR and retry, we will end up in same old state and stuck in loop. As per documentation if Connect() is interrupted by a signal that is caught, while blocked waiting to establish a connection, connect() shall fail and set connect() to [EINTR], but the connection request shall not be aborted, and the connection shall be established asynchronously. When the connection has been established asynchronously, select() and poll() shall indicate that the file descriptor for the socket is ready for writing.
>>
>> Referhttp://www.madore.org/~david/computers/connect-intr.html
> The problem with this change is that currently TRANS_IN_PROGRESS could
> only happen for TRANS_NONBLOCKING sockets, whereas with this change it
> suddenly can happen for normal blocking sockets as well.

For both blocking and non blocking, if connect()interrupted with Signal 
EINTR, it should return TRANS_IN_PROGRESS. When we close the socket 
connection on signal EINTR and retry, we end up in same old state and 
stuck in loop.

In Unix Network Programming, volume 1, section 5.9, W. Richard Stevens 
states:

What we are doing […] is restarting the interrupted system call ourself. 
This is fine for accept, along with the functions such as read, write, 
select and open. But there is one function that we cannot restart 
ourself: connect. If this function returns EINTR, we cannot call it 
again, as doing so will return an immediate error. When connect is 
interrupted by a caught signal and is not automatically restarted, we 
must call "Select" to wait for the connection to complete, as we 
describe in section 15.3.

Referencehttp://www.madore.org/~david/computers/connect-intr.html



> Look for example at the code in libICE/src/connect.c:ConnectToPeer().
> I wouldn't be surprised if the sleep(1) you see there was put in to
> mitigate this problem.  Because of that sleep the connect() is likely
> to succeed the next time around.  But with your change this turns into
> a guaranteed failure!So I don't think you can make this change without making fixes to the
> code that uses this functionality.  Is this code used outside of xorg?
>

I have tested in Solaris and also I can test it in Ubuntu. Even after 
sleep, connect() end up in same old state with signal EINTR. So sleep is 
not solving problem in both blocking and non blocking mode.With or 
without my code changes, connection will close when signal EINTR 
arrives, unless libICE/src/connect.c:ConnectToPeer() handles 
TRANS_IN_PROGRESS state. I have tested Connect() by setting blocking 
mode with _TransSetOption(trans_conn, TRANS_NONBLOCKING, 0);


> In any case, if you make this change, you should also change the
> comments that describe what the current EINTR behaviour.

Yes, I will do it. Request for approving patch I proposed for 
lib/libXfont, code changes are simple and obvious.

Thanks and Regards
-Arvind