Created attachment 190 [details]
test case: multiple applications connect to one sink

TcpSocketImpl forks internally and calls up to the application.  In this case of just a sink, the server ignores this NotifyNewConnectionCreated upcall (which it should, what would it do with the socket it gets?).  See the modified tcp-large-transfer attached for a (admittedly ugly) test case of two applications connecting and sending to a single sink. 

Are you suggesting that the packet-sink needs to maintain a list of accepted sockets perhaps, and some per socket state?  Or does the application itself need to fork for each connection accepted?  To what end, its just a dummy sink with nothing in it but some application level rx tracing you can hook into.

Oh I see the issue now...it is related to calling close correctly.  Yes, the packet sink m_socket is the server socket, you DO need to keep the socket from the NotifyConnectionCreated upcall in some kind of list and call close on each of those...

Created attachment 191 [details]
Add multitasking to PacketSink

The PacketSink keeps references to all of its open sockets, and closes them systematically when StopApplication is closed.  I believe this suffices for multitasking?

Comment on attachment 191 [details]
Add multitasking to PacketSink

># HG changeset patch
># User Raj Bhattacharjea <raj.b@gatech.edu>
># Date 1214951154 14400
># Node ID e1313fb961eb2026771dab041cf02ee4ffe047fc
># Parent  8e6ac6061680a9454d81b65e34be0a7279690bb9
>Multitasking PacketSink
>
>diff -r 8e6ac6061680 -r e1313fb961eb examples/tcp-large-transfer.cc
>--- a/examples/tcp-large-transfer.cc	Tue Jul 01 11:00:29 2008 -0700
>+++ b/examples/tcp-large-transfer.cc	Tue Jul 01 18:25:54 2008 -0400
>@@ -132,6 +132,7 @@ int main (int argc, char *argv[])
> 
>   ApplicationContainer apps = sink.Install (n1n2.Get (1));
>   apps.Start (Seconds (0.0));
>+  apps.Stop (Seconds (60.0));

Can you please create an example where more than one flow terminates on the same packet sink, and verify that all sockets (including the listening) are closed?

> 
>   // Create a source to send packets from n0.  Instead of a full Application
>   // and the helper APIs you might see in other example files, this example
>diff -r 8e6ac6061680 -r e1313fb961eb src/applications/packet-sink/packet-sink.cc
>--- a/src/applications/packet-sink/packet-sink.cc	Tue Jul 01 11:00:29 2008 -0700
>+++ b/src/applications/packet-sink/packet-sink.cc	Tue Jul 01 18:25:54 2008 -0400
>@@ -88,11 +88,17 @@ void PacketSink::StartApplication()    /
>   m_socket->SetRecvCallback (MakeCallback(&PacketSink::HandleRead, this));
>   m_socket->SetAcceptCallback (
>             MakeNullCallback<bool, Ptr<Socket>, const Address &> (),
>-            MakeNullCallback<void, Ptr<Socket>, const Address&> ());
>+            MakeCallback(&PacketSink::HandleAccept, this));
> }
> 
> void PacketSink::StopApplication()     // Called at time specified by Stop
> {
>+  while(!m_socketList.empty()) //these are accepted sockets, close them
>+  {
>+    Ptr<Socket> acceptedSocket = m_socketList.front();
>+    m_socketList.pop_front();
>+    acceptedSocket->Close();
>+  }
>   if (m_socket) 
>     {

     Merge the fix for 243 here that closes m_socket

>       m_socket->SetRecvCallback (MakeNullCallback<void, Ptr<Socket> > ());
>@@ -116,4 +122,9 @@ void PacketSink::HandleRead (Ptr<Socket>
>     }
> }
> 
>+void PacketSink::HandleAccept (Ptr<Socket> s, const Address& from)
>+{
>+  m_socketList.push_back(s);
>+}
>+
> } // Namespace ns3
>diff -r 8e6ac6061680 -r e1313fb961eb src/applications/packet-sink/packet-sink.h
>--- a/src/applications/packet-sink/packet-sink.h	Tue Jul 01 11:00:29 2008 -0700
>+++ b/src/applications/packet-sink/packet-sink.h	Tue Jul 01 18:25:54 2008 -0400
>@@ -81,8 +81,10 @@ private:
>   virtual void StopApplication (void);     // Called at time specified by Stop
> 
>   void HandleRead (Ptr<Socket> socket);
>+  void HandleAccept (Ptr<Socket>, const Address& from);
> 
>   Ptr<Socket>     m_socket;       // Associated socket

Maybe change this comment to // Listening socket

It needs to be a bit more clear that the m_socket is not stored in the m_socketList.

>+  std::list<Ptr<Socket> > m_socketList; //the accepted sockets
>   Address         m_local;        // Local address to bind to
>   TypeId          m_tid;          // Protocol TypeId
>   TracedCallback<Ptr<const Packet>, const Address &> m_rxTrace;

Created attachment 193 [details]
updated fix with extra documentation

This patch is against the latest ns-3-dev (post bug 243 patch) and adds some comments to clarify how the sink operates in regards to its multiple sockets.

(In reply to comment #4)
[snip]
> Can you please create an example where more than one flow terminates on the
> same packet sink, and verify that all sockets (including the listening) are
> closed?

I think that attachment 190 [details] or the new examples/tcp-star-server.cc will suffice for this test case?  What remains to be checked is if the listening socket ever "closes" under the current TCP code, and I'm kind of unclear as to what this would mean in terms of socket behavior and memory management, i.e. what happens (if anything) in terms of TCP when this happens, and should the closed socket get deleted eventually as a result?

Bump.

Does this latest patch look okay?

(In reply to comment #6)
> Bump.
> 
> Does this latest patch look okay?

I believe that the packet sink is supposed to invoke the rx callback for each packet received and you don't do that for the 'accepted' sockets. Or, do you ?

(In reply to comment #7)
> (In reply to comment #6)
> > Bump.
> > 
> > Does this latest patch look okay?
> 
> I believe that the packet sink is supposed to invoke the rx callback for each
> packet received and you don't do that for the 'accepted' sockets. Or, do you ?
> 

I think it should be OK since the HandleRead() method is passed the socket pointer of the socket with data to be read.  Of course, should be tested before checkin.

(In reply to comment #8)

> > I believe that the packet sink is supposed to invoke the rx callback for each
> > packet received and you don't do that for the 'accepted' sockets. Or, do you ?
> > 
> 
> I think it should be OK since the HandleRead() method is passed the socket
> pointer of the socket with data to be read.  Of course, should be tested before
> checkin.

But do you ever connect the HandleRead method to the accepted sockets ? I don't think so. i.e., this:

+void PacketSink::HandleAccept (Ptr<Socket> s, const Address& from)
+{
+  m_socketList.push_back(s);
+}

needs to somehow connect HandleRead to get read events from that socket.

(In reply to comment #9)
> (In reply to comment #8)
> 
> > > I believe that the packet sink is supposed to invoke the rx callback for each
> > > packet received and you don't do that for the 'accepted' sockets. Or, do you ?
> > > 
> > 
> > I think it should be OK since the HandleRead() method is passed the socket
> > pointer of the socket with data to be read.  Of course, should be tested before
> > checkin.
> 
> But do you ever connect the HandleRead method to the accepted sockets ? I don't
> think so. i.e., this:
> 
> +void PacketSink::HandleAccept (Ptr<Socket> s, const Address& from)
> +{
> +  m_socketList.push_back(s);
> +}
> 
> needs to somehow connect HandleRead to get read events from that socket.
> 

I THINK it is already connected to HandleRead through the way TcpSocketImpl uses the copy constructor to fork the accepted connection socket, which copies the callbacks.

I was completely certain it did this before the addition of the extra TcpSocket class in the Socket->TcpSocket->TcpSocketImpl heirarchy.

As always, I'll have a test to verify.

The following is a snippet that verifies that the forked sockets upcall to HandleRead correctly already, and I verified this is due to the compiler generated (POD) copy constructors copying the socket base class callbacks.

@@ -102,6 +108,21 @@ void PacketSink::StopApplication()     /
 
 void PacketSink::HandleRead (Ptr<Socket> socket)
 {
+  if(socket != m_socket)
+    {
+      std::cout<<"HandleRead got an upcall from a forked socket"<<std::endl;
+      std::list<Ptr<Socket> >::iterator i = m_socketList.begin();
+      bool iOwnThisSocket = false;
+      while(i != m_socketList.end())
+        {
+          if (*i == socket)
+            {
+              iOwnThisSocket = true;
+            }
+          ++i;
+        }
+      NS_ASSERT(iOwnThisSocket);
+    }
   Ptr<Packet> packet;
   Address from;
   while (packet = socket->RecvFrom (from))

(In reply to comment #11)
> The following is a snippet that verifies that the forked sockets upcall to
> HandleRead correctly already, and I verified this is due to the compiler
> generated (POD) copy constructors copying the socket base class callbacks.
> 
> @@ -102,6 +108,21 @@ void PacketSink::StopApplication()     /
> 
>  void PacketSink::HandleRead (Ptr<Socket> socket)
>  {
> +  if(socket != m_socket)
> +    {
> +      std::cout<<"HandleRead got an upcall from a forked socket"<<std::endl;
> +      std::list<Ptr<Socket> >::iterator i = m_socketList.begin();
> +      bool iOwnThisSocket = false;
> +      while(i != m_socketList.end())
> +        {
> +          if (*i == socket)
> +            {
> +              iOwnThisSocket = true;
> +            }
> +          ++i;
> +        }
> +      NS_ASSERT(iOwnThisSocket);
> +    }
>    Ptr<Packet> packet;
>    Address from;
>    while (packet = socket->RecvFrom (from))
> 

What seems to be happening is that the automatically generated copy constructor of class Socket is copying the callback.  This is being done implicitly (the copy constructor of TcpSocketImpl does not explicitly copy this callback).

So, it does seem to work, but raises the question of whether it should work this way, implicitly.  A more explicit way to handle this would be to prevent this particular callback (or all callbacks) from being copied implicitly by implementing empty Socket copy constructor, then implement an explicit copy in the application.

The reason that might be preferred is that, it works now, but that is an artifact of the way that PacketSink is implemented.  It might be possible for someone to implement an application for which this implicit copy of this callback might be the wrong thing to do.

I agree this implicit copy could get potentially hairy if you weren't expecting this behavior, just as Mathieu's question illustrates.  Copying the callbacks seemed to be the most natural idea from my perspective, given our choice of using the copy constructor to fork sockets.  Is this something we fix by adding sufficient documentation about the behavior of forking TCP sockets?  People using this code to write TCP apps can always override and set the callbacks themselves if the implicit copy is the wrong thing to do (via Socket::SetRecvCallback).  I think this is as good of an option as any if the user knows to do this, which is why I propose documentation.  It just needs to be clear what the default behavior is and how to change it if you want to.

As a kind of orthogonal issue, the implicitness of all of this code DOES throw me off just a little, so perhaps we should make these copies explicit instead of auto generated anyway?

A discussion between George, Raj, and Tom concluded that the preferred behavior would be to explicitly add this to the TcpSocketImpl copy constructor body:

  SetRecvCallback (MakeNullCallback <void, Ptr<Socket> > ());

and force the application (PacketSink) to then add back the RecvCallback when it is in the HandleAccept routine.

Raj took action item to adjust the patch.

Fixed in ns-3-dev:
3475:8523b98f949c
http://code.nsnam.org/ns-3-dev/rev/8523b98f949c