Bug 205

I looked into this issue again and I have to confess that I still do not really know how to update the current code/API.

1) The current tcp code invokes the close requested callback when it receives a FIN from the remote.

2) In real TCP implementations, if  A calls shutdown, we have the following packet exchange: A -FIN-> B -FIN-> A -ACK-> B. If B was blocked on a read, the read returns EOF when the first FIN is received.

3) In real TCP implementations, shutdown does not (it does not necessarily send the FIN)

To summarize, we need to be notified when we receive a FIN initiated by the other side. Let's call this halfCloseRequested. I see no need for any other callback so, to make progress, I propose that we keep the halfClose callback in SetConnectCallback and remove the others. I have no idea how we should update the current TCP implementation.

Comments ?

I agree with your reasoning, but I am also curious why these callbacks are tacked onto Connect and Accept callback setting routines.   Shouldn't this be split into a separate method (SetHalfCloseRequestedCallback())?

Tom

(In reply to comment #2)
> I agree with your reasoning, but I am also curious why these callbacks are
> tacked onto Connect and Accept callback setting routines.   Shouldn't this be
> split into a separate method (SetHalfCloseRequestedCallback())?

1) I am not suggesting we use any kind of accept callback here
2) it makes sense to associate the half close callback to the connect callback method because half close events will never happen if you are not connected. So, if you want to connect, you must set all callbacks which are related to keeping track of the connection status.

I don't care much if you want to move that callback to a separate method but I see a decent rationale for leaving it with the other connect callbacks.

(In reply to comment #3)

> I don't care much if you want to move that callback to a separate method but I
> see a decent rationale for leaving it with the other connect callbacks.
> 

OK

When I discussed this last with raj, the resolution was that we would remove all close callbacks from the API for this release. Then, we will add the one we need later but this will not be an api breakage, only an extension.

On second though Mathieu, this solution is painful because the Applications depend on getting the closeRequested notification, particularly the packet-sink closes down in response to this notification.  The halfClose isn't used at all in any code, so I will remove this one, and leave the closeRequested (which I suspect can be renamed to match its function; I believe you'd like THIS one to be called halfClose).

Created attachment 167 [details]
Patch that removes the halfClose socket callback

This removes halfClose, but retains closeRequested in its place.  This one doesn't have unclear semantics, and so I elect to not remove it.  The closeCompleted callback doesn't have unclear semantics either, so I also chose to NOT remove it.

There is a CloseUnblocks callback that I added a while back, but it is unused as of yet.  I was envisioning this as the support for the SO_LINGER/close case when close blocks until the FIN is sent on a TCP socket, so the notification would be fired when the FIN hits the wire.  The closeRequested would handle shutdown/close associated FIN from the remote end.

(In reply to comment #6)
> On second though Mathieu, this solution is painful because the Applications
> depend on getting the closeRequested notification, particularly the packet-sink
> closes down in response to this notification.  The halfClose isn't used at all

Can't we just make the PacketSink close the socket from its destructor or dispose method ?

(In reply to comment #8)
> (In reply to comment #6)
> > On second though Mathieu, this solution is painful because the Applications
> > depend on getting the closeRequested notification, particularly the packet-sink
> > closes down in response to this notification.  The halfClose isn't used at all
> 
> Can't we just make the PacketSink close the socket from its destructor or
> dispose method ?
> 

I hadn't thought of that, because it just seemed natural that the sink would shutdown when it knew no more data was coming in.  BUT the code that results from this patch is correct and works.  If anything, it is only misnamed.  I'm not sure what to do though in the interest of moving forward...

(In reply to comment #9)
> > > On second though Mathieu, this solution is painful because the Applications
> > > depend on getting the closeRequested notification, particularly the packet-sink
> > > closes down in response to this notification.  The halfClose isn't used at all
> > 
> > Can't we just make the PacketSink close the socket from its destructor or
> > dispose method ?
> > 
> 
> I hadn't thought of that, because it just seemed natural that the sink would
> shutdown when it knew no more data was coming in.  BUT the code that results
> from this patch is correct and works.  If anything, it is only misnamed.  I'm
> not sure what to do though in the interest of moving forward...

It seems simpler and less risky to remove all the close callbacks. Let's try to do this.

(In reply to comment #9)
> 
> I hadn't thought of that, because it just seemed natural that the sink would
> shutdown when it knew no more data was coming in.  BUT the code that results
> from this patch is correct and works.  If anything, it is only misnamed.  I'm
> not sure what to do though in the interest of moving forward...

Did you try to run the regression tests ? They don't pass anymore and the pcap output is not pretty: I see scores of (retransmitted ?) FINs.

Created attachment 168 [details]
patch to remove all the close callbacks and fix tcp behavior

This is what Mathieu, Tom, and I have agreed to do about this bug.  Craig, please push this patch before you make RC1.  Please also note that it causes a "regression" (the FIN bit isn't set on the last packet in the trace file anymore) so you'll have to update the reference traces.

(In reply to comment #12)
> Created an attachment (id=168) [edit]
> patch to remove all the close callbacks and fix tcp behavior
> 
> This is what Mathieu, Tom, and I have agreed to do about this bug.  Craig,
> please push this patch before you make RC1.  Please also note that it causes a
> "regression" (the FIN bit isn't set on the last packet in the trace file
> anymore) so you'll have to update the reference traces.
> 

I reviewed this just now; did not test this particular patch but came up with something similar earlier today.

Closed with extreme prejudice.