Bug 198

Created attachment 144 [details]
Patch to set the two missing attributes in the TcpSocketImpl copy constructor.

(In reply to comment #0)
> Hello,
> 
> I made a little tcp-echo application (adapted from udp-echo) to test TCP. But I
> face a problem. The TcpSocketImpl::GetTxAvailable() on the echo server always
> returns 0, the server never echoed anything and the TCP stack just send an ACK.
> 
> After inverstigation I noticed in copy constructor (
> TcpSocketImpl::TcpSocketImpl(const TcpSocketImpl& sock) ) that
> the two attributes m_sndBufSize and m_rcvBufSize are not copied.
> 
> In the patch, I set this two attributes in copy constructor and now the server
> echoes the client's packets. 

Sebastien, thanks for this patch.  Raj has the action item to finish off the socket option support for TCP but did not complete it by this weekend.  This is a priority to finish off and push (along with closing out bugs 131 and 166) early next week; your patch can be folded into that.

> 
> I think it is normal that the echo-response does not set the ACK flags as
> NS-3's TCP stack does not implement the delayed acknowledgement (which is why
> the server send an ACK after the echo response).
> 

ns-3 TCP should be sending delayed acks:

  // Now send a new ack packet acknowledging all received and delivered data
  if(++m_delAckCount >= m_delAckMaxCount)
  {
    m_delAckEvent.Cancel();
    m_delAckCount = 0;
    SendEmptyPacket (TcpHeader::ACK);
  }
  else
  {
    m_delAckEvent = Simulator::Schedule (m_delAckTimeout, &TcpSocketImpl::DelAckTimeout, this);
  }

and the output can be seen in the TCP example file, but if you think it is not behaving properly, please file a bug on it.

(In reply to comment #2)
> (In reply to comment #0)
> > Hello,
> > 
> > I made a little tcp-echo application (adapted from udp-echo) to test TCP. But I
> > face a problem. The TcpSocketImpl::GetTxAvailable() on the echo server always
> > returns 0, the server never echoed anything and the TCP stack just send an ACK.
> > 
> > After inverstigation I noticed in copy constructor (
> > TcpSocketImpl::TcpSocketImpl(const TcpSocketImpl& sock) ) that
> > the two attributes m_sndBufSize and m_rcvBufSize are not copied.
> > 
> > In the patch, I set this two attributes in copy constructor and now the server
> > echoes the client's packets. 
> 
> Sebastien, thanks for this patch.  Raj has the action item to finish off the
> socket option support for TCP but did not complete it by this weekend.  This is
> a priority to finish off and push (along with closing out bugs 131 and 166)
> early next week; your patch can be folded into that.
> 
> > 
> > I think it is normal that the echo-response does not set the ACK flags as
> > NS-3's TCP stack does not implement the delayed acknowledgement (which is why
> > the server send an ACK after the echo response).
> > 
> 
> ns-3 TCP should be sending delayed acks:
> 
>   // Now send a new ack packet acknowledging all received and delivered data
>   if(++m_delAckCount >= m_delAckMaxCount)
>   {
>     m_delAckEvent.Cancel();
>     m_delAckCount = 0;
>     SendEmptyPacket (TcpHeader::ACK);
>   }
>   else
>   {
>     m_delAckEvent = Simulator::Schedule (m_delAckTimeout,
> &TcpSocketImpl::DelAckTimeout, this);
>   }
> 
> and the output can be seen in the TCP example file, but if you think it is not
> behaving properly, please file a bug on it.
> 

Hum in fact the problem is that in wireshark I see "Duplicate ACK", I follow the tcp-socket-impl code and if I call SendPendingData() with its parameters to true (called withAck) the behavior is correct (ACK sent with data).

I am not a "master of TCP" so I don't know if doing this is right in the code. Maybe Raj could see if it is good.

Here the diff (the first part is the previous missing initialization in copy constructor) : 

diff -r 61fe7fe81ebd src/internet-node/tcp-socket-impl.cc
--- a/src/internet-node/tcp-socket-impl.cc	Thu May 29 23:24:10 2008 -0700
+++ b/src/internet-node/tcp-socket-impl.cc	Sat May 31 17:00:44 2008 +0200
@@ -126,7 +126,9 @@ TcpSocketImpl::TcpSocketImpl(const TcpSo
     m_cnTimeout (sock.m_cnTimeout),
     m_cnCount (sock.m_cnCount),
     m_rxAvailable (0),
-    m_wouldBlock (false) 
+    m_wouldBlock (false),
+		m_rcvBufSize(sock.m_rcvBufSize),
+		m_sndBufSize(sock.m_sndBufSize)
 {
   NS_LOG_FUNCTION_NOARGS ();
   NS_LOG_LOGIC("Invoked the copy constructor");
@@ -655,7 +657,7 @@ bool TcpSocketImpl::ProcessAction (Actio
       break;
     case TX_DATA:
       NS_LOG_LOGIC ("TcpSocketImpl " << this <<" Action TX_DATA");
-      SendPendingData ();
+      SendPendingData (true);
       break;
     case PEER_CLOSE:
       NS_ASSERT (false); // This should be processed in ProcessPacketAction

The copy constructor issue is resolved in
3238:ec45f705b9ca
3242:19471cb55c9c

Of raj/ns-3-dev-socket-helper

On the other issue of piggybacking ACKs on data...I'm a bit dubious about the solution because putting the "true" parameter where Sebastien suggests unconditionally sets the ack on every outgoing packet.  I'm not sure this is the right behavior...Sebastien, have you tested some other TCP test cases with this logic?

Since the copying issue is resolved, I renamed the bug to reflect the open issue.

(In reply to comment #5)
> Since the copying issue is resolved, I renamed the bug to reflect the open
> issue.
> 

What is the current open issue?  Can a test case and some detail be provided?

(In reply to comment #6)
> 
> What is the current open issue?  Can a test case and some detail be provided?
> 

The issue is that our TCP implementation doesn't send ACK packets along with any return application data.  Sebastien's echo responses don't piggyback an ACK on them acknowledging the last received segment.  Our TCP simply sends an explicit ACK and any return data separately.

As for a test case, Sebastien's original one would be best, but I think David Evensky's TCP two-way could be made to do this.  This isn't so much a bug as an enhancement at this point, because the behavior is correct, just non optimal from a network-level protocol overhead perspective.  As such I'm reducing the priority and severity.

Created attachment 165 [details]
tcp-echo application and example file

Sorry for the late reply! But here are the tcp-echo application and an example file to test.

(In reply to comment #8)
> Created an attachment (id=165) [edit]
> tcp-echo application and example file
> 
> Sorry for the late reply! But here are the tcp-echo application and an example
> file to test.
> 

Unless I'm mistaken, the patch you provided only changes the wscript files!  It doesn't contain a diff for the actual cc file.

Created attachment 166 [details]
A tcp-echo application and example file.

Oups, sorry! Here are the correct diff with the .cc/.h files.

Fixed in ns-3-dev
4269:78d709bebd55