http://code.nsnam.org/raj/ns-3-tcp/rev/83ccf9c5afa8

Does this do the trick?

The inline docs say:

"""Sets the "close" callback, which is a notification
that the other end of the connection has sent a message
indicating that it will not send any more data, i.e.
the socket is closed for reading."""

While this may be true, I think it's even more important to note that, when the close callback is invoked we have a guaranteed that all data _we transmitted_ has been flushed and acknowledged by the receiver.

Otherwise looks good.

(In reply to comment #1)
> http://code.nsnam.org/raj/ns-3-tcp/rev/83ccf9c5afa8
> 
> Does this do the trick?
> 

I do not think the implementation of this yields desired semantics.  I think we want to be able to provide a signal that the application has read through the received data up to a received FIN segment.

from man 3 recv:

"Upon successful completion, recv() shall return the length of the message in bytes. If no messages are available to be received and the peer has performed an orderly shutdown, recv() shall return 0. Otherwise, -1 shall be returned and errno set to indicate the error. "

If you call this when the FIN is received, but you do not first make sure that the application has read all of the locally received data, it is not equivalent to a recv() that returns 0.  FIN may also be received out of order.

So, if we agree that we want to support the above recv() semantics (it is not clear from the initial bug report), I would suggest to change it and change the doxygen to say that it is supposed to align with the case that a synchronous socket read() returns zero.

(In reply to comment #2)
> The inline docs say:
> 
> """Sets the "close" callback, which is a notification
> that the other end of the connection has sent a message
> indicating that it will not send any more data, i.e.
> the socket is closed for reading."""
> 
> While this may be true, I think it's even more important to note that, when the
> close callback is invoked we have a guaranteed that all data _we transmitted_
> has been flushed and acknowledged by the receiver.

We would need to be notified in this case of an ack of our FIN, or detection of RSTs, which is not what this is doing.

It would help to clarify what we want to model; I can't really tell from the initial bug report.  

(In reply to comment #2)
> The inline docs say:
> 
> """Sets the "close" callback, which is a notification
> that the other end of the connection has sent a message
> indicating that it will not send any more data, i.e.
> the socket is closed for reading."""
> 
> While this may be true, I think it's even more important to note that, when the
> close callback is invoked we have a guaranteed that all data _we transmitted_
> has been flushed and acknowledged by the receiver.

If we want to be able to convey that all data we transmitted has been successfully sent, we could do two things that should cover most cases:

- provide some equivalent to SO_LINGER socket option, with a callback for notifying the application that close() would have returned, for apps that use close() after write()
- suggest to users the following socket call sequence:

Set the close callback
when done writing, call ShutdownSend()
wait for NotifyClose()
then call Close()

as in: http://blog.netherlabs.nl/articles/2009/01/18/the-ultimate-so_linger-page-or-why-is-my-tcp-not-reliable

we should also make sure that there is a means to reliably detect when the peer has sent a RST

From the article Tom posted:
What we can do however is issue a shutdown(), which will lead to a FIN packet being sent to program B. Program B in turn will close down its socket, and we can detect this from program A: a subsequent read() will return 0.


This scenario (of detecting EOF when reading from the socket) is supported by the current TCP implementation; the packet you get in the RecvCallback will have Size=0 only when the read reaches EOF, which is when the read hits the data that had the FIN on it originally.

So the question becomes making sure Shutdown's semantics are correct?  Accordingly, Tom's suggestion for how to close a socket becomes:

when done writing, call ShutdownSend()
wait for RecvCallback to give an empty packet; then you know the other end has sent a FIN too;
call close() (which might be meaningless in ns-3, since in *NIX, calling it at this point it tantamount to kernel resource cleanup, it shouldn't have any network effect at all)

(In reply to comment #1)
> http://code.nsnam.org/raj/ns-3-tcp/rev/83ccf9c5afa8
> 
> Does this do the trick?
> 

(In reply to comment #6)
> From the article Tom posted:
> What we can do however is issue a shutdown(), which will lead to a FIN packet
> being sent to program B. Program B in turn will close down its socket, and we
> can detect this from program A: a subsequent read() will return 0.
> 
> 
> This scenario (of detecting EOF when reading from the socket) is supported by
> the current TCP implementation; the packet you get in the RecvCallback will
> have Size=0 only when the read reaches EOF, which is when the read hits the
> data that had the FIN on it originally.

Raj, it seems you are saying that the originally reported bug (callback needed) is invalid since the read of length zero suffices?  What is the immediate plan for this bug?  

It seems invalid to me.  For refactoring close/shutdown (which is closely tied to the ideas in this bug) see the patch for bug 426.

I think there are two problems identified in the comments below.

Problem 1) if the peer closes and sends a RST, then the sender should be able to detect this.  In a normal implementation, recv () would return -1 and errno would be set to ECONNRESET.  In ns-3, using the Ptr<Packet>-based API for receive, there is no way to convey this presently.  Using the byte-based recv() API that returns an int, there still is not a way to convey this because that method simply calls the Ptr<Packet>-based method.

A couple of options to fix the above API problem:
1) modify existing NotifyDataRecv() callback in Socket base class as follows:
- void SetRecvCallback (Callback<void, Ptr<Socket> >);
+ void SetRecvCallback (Callback<void, Ptr<Socket>, int >);
where the int can be used to signal "-1" to the function handling recv callbacks
2) add a new callback such as RecvError callback that is invoked whenever read would have returned -1 in a sockets implementation
  
I think that this bug could be renamed:  TCP RST notification needed

Problem 2) People seem to want a callback that mimics a blocking close with SO_LINGER semantics.

For this, I think this is not a bug but a feature request to add SO_LINGER, and some kind of a CloseCallback that signals when the system would have either returned from a lingering close or timed out.  For this, a new bug could be opened.

Created attachment 610 [details]
Correct broken close semantics/implementation

Fixes several problems with TCP close semantics.  Appears to fix bugs
326, 559, 663, and 664 also.

(In reply to comment #10)
> Created an attachment (id=610) [details]
> Correct broken close semantics/implementation
> 
> Fixes several problems with TCP close semantics.  Appears to fix bugs
> 326, 559, 663, and 664 also.
> 

I think that the newly proposed socket API is necessary.  Because it isn't plumbed in for NSC yet, and because it is too close to code freeze, I suggest to split this patch into two patches; one dealing with TCP internal closing fixes (that also fixes other TCP bugs) and one dealing with the two new socket callbacks, and keep this open and at P2, and that we resolve this in the next month including for NSC.

One question I have concerns when NotifyNormalClose should be called.  The proposed patch calls it when the TCP receiver has received the FIN in sequence (or plugs all remaining sequence gaps) but is not tied to whether the application has read through to EOF.  Another possible implementation would call it only after the application has read through to EOF.  

changeset c54b36fb7317