Bug 132 - TCP server sockets do not fork on new connections
: TCP server sockets do not fork on new connections
Status: RESOLVED FIXED
: ns-3
internet-stack
: pre-release
: All All
: P3 major
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2008-02-06 10:24 EDT by
Modified: 2008-03-27 10:45 EDT (History)


Attachments
Patch to notify app of the new connection (945 bytes, patch)
2008-02-13 09:59 EDT, Rajib Bhattacharjea
Details | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2008-02-06 10:24:34 EDT
The TcpSocket class never calls NotifyNewConnectionCreated, and so server-side
sockets accept connections allright but don't notify the user code that a new
socket has been created.  Without this it's impossible to receive data inside a
 server application, as far as i can tell...
------- Comment #1 From 2008-02-06 12:38:16 EDT -------
This is part of the larger issue that listening server sockets don't actually
spawn new sockets/endpoints for accepted connections, but rather simply
communicate over the formerly listening socket.  On today's call, we committed
to having the correct behavior by the end of the week.
------- Comment #2 From 2008-02-06 12:49:07 EDT -------
(In reply to comment #1)
> This is part of the larger issue that listening server sockets don't actually
> spawn new sockets/endpoints for accepted connections, but rather simply
> communicate over the formerly listening socket.  On today's call, we committed
> to having the correct behavior by the end of the week.

OK... it would be nice to have a complete TCP for the next ns-3 release. 
Otherwise the release notes should have a warning about this issue, to prevent
users from spending time trying to look for bugs in their own code when it's
simply a feature not implemented in ns-3 :-/
------- Comment #3 From 2008-02-08 17:54:52 EDT -------
Implemented in tree:
http://code.nsnam.org/raj/ns-3-dev/

Quick link to the file:
http://code.nsnam.org/raj/ns-3-dev/file/d66ffafd78c9/src/internet-node/tcp-socket.cc

The basic logic is that when a SYN comes in, the listening socket performs a
stateful clone of itself using the copy constructor (eventually the "virtual
constructor"/copy/clone pattern, but not yet), and then calls into the newly
cloned socket to send back the reply and notify the application that a
connection was requested.  When it completes the handshake, the cloned socket
fires a notify up to the application that a new socket has been created and
passes up itself and the remote end's IP address.

There are some memory management/smart pointer issues to figure out yet (very
likely that the refcount of the cloned socket is not correct), but the code
appears to work albeit with leaks.  See examples/tcp-full-duplex.cc in my tree
from above as it is the only TCP example scripts that uses the newly
implemented semantics.  All the rest need slight modification to support the
change.  I'll try to get this done over the weekend.
------- Comment #4 From 2008-02-11 00:11:30 EDT -------
I think there are two issues; a simple one and a harder one.  The simple one is
to simply fix the behavior for a non-multitasking server.  This should be
fixable with minimal effort (Gustavo's initial comment).

The second one is the multitasking server.  Here, there may need to be more
work done because tcp-full-duplex.cc (or any of our applications) do not even
handle the forking sockets because they themselves do not fork.

So I'd suggest to fix this one immediately without waiting to finalize the
forking server case.
------- Comment #5 From 2008-02-12 10:00:57 EDT -------
(In reply to comment #4)
> I think there are two issues; a simple one and a harder one.  The simple one is
> to simply fix the behavior for a non-multitasking server.  This should be
> fixable with minimal effort (Gustavo's initial comment).
> 
> The second one is the multitasking server.  Here, there may need to be more
> work done because tcp-full-duplex.cc (or any of our applications) do not even
> handle the forking sockets because they themselves do not fork.
> 
Our TCP sockets DO fork now whenever a SYN packet arrives.  I'm not sure if I
made this clear or not in the original post, but what is in
http://code.nsnam.org/raj/ns-3-dev forks the socket and returns it to the
application via a call to NotifyNewConnectionCreated.  The original server
socket keeps on listening for incoming SYNs.  I'm not sure I understand what
the two issues are?
------- Comment #6 From 2008-02-12 13:52:58 EDT -------
Surprisingly, our current test cases with one sender and one packet-sink works
with the change to forked sockets.  The reason is that the packet sink
registers a receive callback with its listening socket.  When the listening
socket is forked, the copy constructor copies the callback as well, and then
invokes it upon data receipt, passing up to the application correctly.

More testing remains to be done to see if the behavior is correct for multiple
connections to one server.
------- Comment #7 From 2008-02-13 00:48:44 EDT -------
(In reply to comment #5)
> (In reply to comment #4)
> > I think there are two issues; a simple one and a harder one.  The simple one is
> > to simply fix the behavior for a non-multitasking server.  This should be
> > fixable with minimal effort (Gustavo's initial comment).
> > 
> > The second one is the multitasking server.  Here, there may need to be more
> > work done because tcp-full-duplex.cc (or any of our applications) do not even
> > handle the forking sockets because they themselves do not fork.
> > 
> Our TCP sockets DO fork now whenever a SYN packet arrives.  I'm not sure if I
> made this clear or not in the original post, but what is in
> http://code.nsnam.org/raj/ns-3-dev forks the socket and returns it to the
> application via a call to NotifyNewConnectionCreated.  The original server
> socket keeps on listening for incoming SYNs.  I'm not sure I understand what
> the two issues are?
> 

The two issues are 
i) Gustavo's post that the current non-multitasking code needs fixed
ii) Your suggestion that this is part of a broader issue with implementing
server forks

If ii) is not ready for this release, I suggest to address i) for this release
------- Comment #8 From 2008-02-13 09:59:43 EDT -------
Created an attachment (id=107) [details]
Patch to notify app of the new connection

This does the correct notification up to the application of the new connection.
 The server fork isn't quite ready yet for merging, so I'll merge this patch in
before the release instead to address Gustavo's original issue.
------- Comment #9 From 2008-02-14 11:20:41 EDT -------
Patch pushed. (http://code.nsnam.org/ns-3-dev/rev/6ae8117d4ffe)

The issue of server forks is still open.  It has been implemented (but not
thorougly tested) in http://code.nsnam.org/raj/ns-3-dev/
------- Comment #10 From 2008-02-14 15:06:08 EDT -------
Changed the name of the bug to address the broader issue.
------- Comment #11 From 2008-03-27 10:45:44 EDT -------
Fixed in ns-3-dev, changesets 2362-2368.