Proposal to change TIPC poll() behavior

Hi there:

I finally got around to looking at Tamas's patch from late last year, and found something that caused me to
take a closer look at the way TIPC 1.7 sets flags for poll().  In doing so, I think I found some other problems
with the way TIPC sets these flags under other conditions.  After continued research into this sometimes
confusing area, I think I've come up with an approach that might resolve all of these issues.

The Problem
===========
Tamas' patch was intended to ensure that a newly created connection-oriented socket did not indicate
POLLIN, since there was not actually any data present on the socket to be read.  (Fair enough.)  However, I
also found other cases where flags might be set unexpectedly, specifically:

- a socket that was in the "connecting" state might indicate POLLIN, due to the presence of a transient ACK
message sitting in its socket receive queue, and

- a socket that was in the "listening", "unconnected", or "connecting" state would always indicate
POLLOUT, since such sockets are never prevented from sending due to port congestion.

Hopefully, others will agree that these behaviors are incorrect.  If not, please speak up!

The Proposed Solution
=====================
While these bugs don't seem to be causing problems at the moment, it's probably only because TIPC users
aren't writing applications that check for them.  (For example, why would anyone want to check a listening
socket to see if it was writable?)  However, as Tamas' case illustrates, if you wait long enough someone
will eventually try using TIPC sockets in unusual ways and bugs like these might be exposed.

What I am proposing is that TIPC set flags for poll() as follows:

socket state	flags set
------------	---------
unconnected		none

connecting		none

connected		POLLIN (if data in rx queue); POLLOUT (unless port congestion is present)

disconnecting	POLLIN; POLLHUP

listening		POLLIN (if connection requeust in rx queue)

ready, aka
connectionless	POLLIN (if data in rx queue); POLLOUT (since port congestion can't happen)

I don't think that there should be much debate about most of these cases.  It seems clear to me that an
unconnected socket is neither readable nor writable, and that the same is true for a socket that is in the
process of establishing a connection.  (I should point out that TIPC doesn't support asychronous
connect(), so there is no need to set POLLIN to indicate that a connection is now established.)  The rest
will hopefully be similarly self-evident, with the possible exception of the treatment of
"disconnecting"; in this case I am proposing that TIPC set POLLIN under all conditions -- even if there are
no messages actually in the socket's rx queue at the time the connection is broken.  There are several
reason's for this:

- this seems to be a Linux convention (but is by no means a standard across all Unix-like implementations),
- I've seen claims that not setting the flag could cause applications that only look for POLLIN to hang if a
connection breaks when the socket's queue is empty, and
- this is already the way TIPC does things and it's less risky to leave things as they are ...

Anyway, that's where things currently stand.  I'd be interested in hearing back from people regarding this
proposal before I go ahead and start coding it up.

Regards,
Al

> -----Original Message-----
> From: Tamas Morvai [mailto:Tamas.Morvai <at> ericsson.com] 
> Sent: Tuesday, December 01, 2009 8:47 AM
> To: Stephens, Allan
> Cc: Jon Maloy; tipc-discussion <at> lists.sourceforge.net
> Subject: Re: TIPC poll
> 
> Hi,
> 
> The patch is in the attachment.
> Setting POLLIN is removed from the SS_UNCONNECTED and the 
> SS_DISCONNECTING state. POLLIN should only be set if there is 
> data in the buffer otherwise a well behaving client (like our 
> vDicos) has to call recv which returns ENOTCONN. Obviously, 
> calling recv, if there is no data, is unnecessary and should 
> be avoided.
> 
> /Tamas
> 
> 
> Stephens, Allan wrote:
> > Hi Tamas:
> > 
> > Your concerns were discussed at yesterday's TIPC Working 
> Group meeting, and it seems that no one has any objections to 
> modifying TIPC 1.7 to try and eliminate the undesirable 
> behavior you've identified.  Can you post a patch to the TIPC 
> discussion mailing list (copied on this email) so it can be 
> reviewed (and hopefully accepted)?
> > 
> > Regards,
> > Al
> > 
> >> -----Original Message-----
> >> From: Tamas Morvai [mailto:Tamas.Morvai <at> ericsson.com]
> >> Sent: Friday, November 20, 2009 4:08 AM
> >> To: Stephens, Allan
> >> Cc: Jon Maloy
> >> Subject: Re: TIPC poll
> >>
> >>
> >>
> >> Stephens, Allan wrote:
> >>> Hi there:
> >>>
> >>> Currently, it looks like both TIPC 1.6 and TIPC 1.7 treat
> >> POLLIN the
> >>> same way.  By design, we took the approach of setting
> >> POLLIN whenever
> >>> a connection-based socket was in a state in which a read 
> operation 
> >>> could be performed without blocking, and thus POLLIN is 
> set if you 
> >>> have an unconnected socket.
> >>>
> >>> What I don't understand at the moment is why the existing
> >> arrangement
> >>> isn't satisfactory.  As far as I can tell, when you create a 
> >>> connection-based socket you are doing one of three things:
> >>>
> >>> 1) creating a socket which you want to use to do connect(), or 2) 
> >>> creating a socket which you want to use to do accept(), or
> >> 3) you get
> >>> a socket created for you by an accept() operation.
> >>>
> >>> In the first case you can avoid undesired POLLIN indications by 
> >>> issuing a connect() before you add the socket to the list
> >> of sockets
> >>> monitored by poll().  Similarly, in the second case you can
> >> avoid the
> >>> problem by calling listen() before poll().  In the final
> >> case you can
> >>> call poll() right away, as the socket is already connected.
> >>>
> >>> The only way I can see you having a problem is if you
> >> create a socket
> >>> for 1) or 2) that you start monitoring before you use 
> connect() or 
> >>> listen(); however, in that case I don't understand why 
> you'd bother 
> >>> creating the socket at all.  Maybe I'm missing something here ...
> >> Yes and this is what we do to make matters simple. It is 
> much easier 
> >> from a design perspective to start monitoring in the ctor than 
> >> explicitly start it at connect or listen. From a 
> programming point of 
> >> view it makes more sence to do this monitoring stuff in 
> the ctor and 
> >> dtor of our Socket class.
> >>
> >> We use async sockets, so connect can return EAGAIN in 
> which case we 
> >> have to start polling and if the connect fails we have to stop 
> >> polling. Why should we make matters more complicated than it is 
> >> necessary?
> >>
> >> If other socket types work normally than why does not TIPC?
> >> I quite expect all socket types to work in the same way.
> >>
> >> Just for the record I already modified our code as you 
> suggested, but 
> >> I would really like to revert these modifications. However, as I 
> >> wrote vDicos will be a _big_ TIPC user. I am quite sure nobody has 
> >> used TIPC so heavily as vDicos will. I was just asking why this 
> >> feature had been implemented in the first place. As far as I am 
> >> concerned this is an unexpected behaviour from a socket.
> >>
> >> /Tamas
> >>
> >>> Regards, Al
> >>>
> >>>> -----Original Message----- From: Jon Maloy 
> >>>> [mailto:jon.maloy <at> ericsson.com] Sent: Thursday, November 19, 2009
> >>>> 10:31 AM To: Stephens, Allan Cc: Tamás Morvai Subject: FW: 
> >> TIPC poll
> >>>>
> >>>> Hi Allan, See below. I believe you wrote this code, and
> >> know if this
> >>>> has been fixed in tipc-1.7.
> >>>>
> >>>> Regards ///jon
> >>>>
> >>>> Jon Maloy M.Sc. EE Researcher
> >>>>
> >>>> Ericsson Canada Inc. Broadband and Systems Research 8400, boul 
> >>>> Décarie Ville Mont-Royal, QC, H4P 2N2 Canada Téléphone: 
> +1 514 345 
> >>>> 7900 ext 42056 Cellulaire: +1 514 591 5578 jon.maloy <at> ericsson.com
> >>>>
> >>>> -----Original Message----- From: Tamás Morvai Sent: 
> November-19-09
> >>>> 04:11 To: Jon Maloy Subject: Re: TIPC poll
> >>>>
> >>>> Hi Jon,
> >>>>
> >>>> The open source version which comes with SLES 10.
> >>>>
> >>>> /Tamas
> >>>>
> >>>> Jon Maloy wrote:
> >>>>> Hi Tamas, Which TIPC version are you using? The open
> >> source version
> >>>> at SourceForge? The one embedded in the Linux kernel 
> (which lags a 
> >>>> generation behind the SF version), or the Ericsson
> >> internal one, used
> >>>> in Dicos and TSP?
> >>>>> Regards ///jon
> >>>>>
> >>>>>
> >>>>> Jon Maloy M.Sc. EE Researcher
> >>>>>
> >>>>> Ericsson Canada Inc. Broadband and Systems Research 8400, boul 
> >>>>> Décarie Ville Mont-Royal, QC, H4P 2N2 Canada Téléphone: +1 514
> >>>>> 345 7900 ext 42056 Cellulaire: +1 514 591 5578 
> >>>>> jon.maloy <at> ericsson.com
> >>>>>
> >>>>>
> >>>>>>> -----Original Message----- From: Tamás Morvai Sent:
> >>>>>>> November-18-09 06:57 To: Jon Maloy Subject: TIPC poll
> >>>>>>>
> >>>>>>> Hi Jon,
> >>>>>>>
> >>>>>>> Why does TIPC set EPOLLIN if a connection-oriented socket
> >>>> does not
> >>>>>>> have an active connection? Just for the sake of an
> >>>> unblocking read?
> >>>>>>> Is there any application which depends on this behaviour? 
> >>>>>>> Currently, we are developing vDicos (you may have heard about
> >>>>>>>  it) and vDicos will be a _big_ TIPC user. The problem is
> >>>> that we add
> >>>>>>> the fd to epoll at socket creation to make matters simple
> >>>> with the
> >>>>>>> level-trigger option and we constantly receive EPOLLIN from 
> >>>>>>> epoll_wait which is bad for us.
> >>>>>>>
> >>>>>>> A non-TIPC socket works as we expect it (no EPOLLIN event). 
> >>>>>>> Is there a way you can fix this for us?
> >>>>>>>
> >>>>>>> /Tamas
> >>>>>>>
> 

------------------------------------------------------------------------------
The Planet: dedicated and managed hosting, cloud storage, colocation
Stay online with enterprise data centers and the best network in the business
Choose flexible plans and management services without long-term contracts
Personal 24x7 support from experience hosting pros just a phone call away.
http://p.sf.net/sfu/theplanet-com