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 an attachment (id=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 an attachment (id=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] [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.
> 

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

Closed with extreme prejudice.