I'm towards accepting the patch as-is. Any further comments?

I normally see the empty ACK in real flows; why are we deleting it here?

The node that has data to send will send the initial SYN.  The typical pattern is:

S-->
 <--- S,A
A--->
DATA-->

If I understand correctly, the patch will eliminate the third packet above.

(In reply to Tom Henderson from comment #2)
> If I understand correctly, the patch will eliminate the third packet above.

Exactly .. but I never understood why it is not a good practice to piggy-back some data into that ACK. 

In general we are not good into piggybacking data; see for instance each time we send a duplicate ACK. Reading RFC 6675, that defines dupack something that updates the scoreboard (and so, the requirement packetsize = 0 has gone), is made me thinking that we can piggy back data practically everywhere.

(In reply to natale.patriciello from comment #3)
> (In reply to Tom Henderson from comment #2)
> > If I understand correctly, the patch will eliminate the third packet above.
> 
> Exactly .. but I never understood why it is not a good practice to
> piggy-back some data into that ACK. 
> 
> In general we are not good into piggybacking data; see for instance each
> time we send a duplicate ACK. Reading RFC 6675, that defines dupack
> something that updates the scoreboard (and so, the requirement packetsize =
> 0 has gone), is made me thinking that we can piggy back data practically
> everywhere.

I always love to dig into the RFCs, it avoids "guesses". As a consequence, I'll cite RFC 793, section 3.4:

  Several examples of connection initiation follow.  Although these
  examples do not show connection synchronization using data-carrying
  segments, this is perfectly legitimate, so long as the receiving TCP
  doesn't deliver the data to the user until it is clear the data is
  valid (i.e., the data must be buffered at the receiver until the
  connection reaches the ESTABLISHED state).

The RFC is clear. You CAN send data during the 3-way handshake, in both directions.
The fact that this is often not done is a different problem.

Note that we should also check what is the effect of TCP Syn Cookies (rfc6013), although also in that RFC is stated that "The cookie exchange may carry data"

> 
> The RFC is clear. You CAN send data during the 3-way handshake, in both
> directions.
> The fact that this is often not done is a different problem.
> 

I think the goal should be to model what real-world TCPs do, not to try to optimize our implementation.  I have always seen the zero-length ack in real traces, so I would rather keep it until someone provides trace evidence that TCPs have started to do this piggybacking.

I did some heavy digging on this, mostly because I'm curious.
My further conclusion is: it IS possible to send data on the 1st ACK, it's just unlikely.

A reference:
https://labs.ripe.net/Members/gih/the-curious-case-of-the-crooked-tcp-handshake
Here Geoff Huston explicitly considers the case of data on 1st ACK, and I trust him more than a book.

Second point: a "send" call performed on a non-connected TCP socket will always return an error of type "Socket is not connected". However, from the client's point of view the socket is connected just after receiving the SYN-ACK.

The problem (if it's a problem) is that nowadays the TCP_QUICKACK option is set to on (see https://news.ycombinator.com/item?id=10607422).
As a consequence, TCP will just reply to the SYN-ACK with an empty ACK, just because the application didn't had the time to fill the buffer - call it a race condition between sending an ACK and firing a callback to un-block the blocking socket and/or to signal the non-blocking socket that something happened.

And not, what about the ns-3 implementation ?

Well, point 1: the user shouldn't be able to use a Send if the socket is not connected. This is obvious. Data should be discarded and an error should be raised.
Point 2: if (somehow) TCP finds some data in the Tx buffer when it have to send the first ACK, then it's totally legal to send the the ACK piggybacked.

What is not wise (legal, but not wise) is to use delayed ACKs along with Nagle.




(In reply to Tom Henderson from comment #5)
> > 
> > The RFC is clear. You CAN send data during the 3-way handshake, in both
> > directions.
> > The fact that this is often not done is a different problem.
> > 
> 
> I think the goal should be to model what real-world TCPs do, not to try to
> optimize our implementation.  I have always seen the zero-length ack in real
> traces, so I would rather keep it until someone provides trace evidence that
> TCPs have started to do this piggybacking.

(In reply to Tommaso Pecorella from comment #6)
> And not, what about the ns-3 implementation ?
> 
> Well, point 1: the user shouldn't be able to use a Send if the socket is not
> connected. This is obvious. Data should be discarded and an error should be
> raised.
> Point 2: if (somehow) TCP finds some data in the Tx buffer when it have to
> send the first ACK, then it's totally legal to send the the ACK piggybacked.
> 
> What is not wise (legal, but not wise) is to use delayed ACKs along with
> Nagle.

Currently, we store data in SYN_SENT state:

if (m_state == ESTABLISHED || m_state == SYN_SENT || m_state == CLOSE_WAIT)
    {
      // Store the packet into Tx buffer
      if (!m_txBuffer->Add (p))

(tcp-socket-base.cc:824). Is this a bug? If we assume TCP is connected only after entering ESTABLISHED, yes.

What we should do with this bug ?