The expectation is that sockets are held by Applications which are held by Nodes. So, if you want to avoid these problems, the best way to do so is by providing new subclasses of the Application base class which hold your sockets.

Holding sockets in some class, be it Application or otherwise, is more work than needed.  NS-3 can save the programmer work by doing this work by itself, with little cost.

Actually the cost here is _discovering_ the problem.  It wasn't at all obvious, even for me, and only through debugging with NS_LOG was I able to track it down.  The solution to problem is easy; diagnosing the problem was hard.

Eliminating nasty surprises in the simulator is part of our job.  So I strongly recommend either make  it work (preferably) or detect the problem automatically and give an error message.  I only know how to make it work, not detect.

(In reply to comment #2)
> Holding sockets in some class, be it Application or otherwise, is more work
> than needed.  NS-3 can save the programmer work by doing this work by itself,

I understand you feel pretty strongly that this is a user bug which can be avoided but,

> with little cost.

The memory cost is not "little" so, I am very uneasy about this proposal, especially since there is another fix, as outlined in my previous comment. I am also pretty uneasy about this proposed change because I really think that it is a bug to create a socket, release it, and expect it to stay around while you run the simulation. 

Furthermore, I have a very hard time to see how your proposal would gracefully deal with appearing and disappearing sockets: how can you (user) make sure that a socket is really released ? Do you have to go into the array attached to the node and remove it from there ? _that_ sounds really bad.

(In reply to comment #3)
> (In reply to comment #2)
> > Holding sockets in some class, be it Application or otherwise, is more work
> > than needed.  NS-3 can save the programmer work by doing this work by itself,
> 
> I understand you feel pretty strongly that this is a user bug which can be
> avoided but,
> 
> > with little cost.
> 
> The memory cost is not "little" so, I am very uneasy about this proposal,
> especially since there is another fix, as outlined in my previous comment. I am
> also pretty uneasy about this proposed change because I really think that it is
> a bug to create a socket, release it, and expect it to stay around while you
> run the simulation. 

The memory cost is one pointer per socket.  Surely that is negligible.  Same problem as keeping a list of applications in the Node.

> 
> Furthermore, I have a very hard time to see how your proposal would gracefully
> deal with appearing and disappearing sockets: how can you (user) make sure that
> a socket is really released ? Do you have to go into the array attached to the
> node and remove it from there ? _that_ sounds really bad.

Like I said, I would envision socket->Close() to take care of it.  Or even if a socket is closed from the remote side, the Node could then release its reference to it (since a closed socket cannot be used for communication anymore).

I agree that it is not really intuitive the way the code now works, as exemplified by this test case.  I as a user would expect that if I used a socket and passed data to it, I could delete my local reference to the socket afterwards, just as I could lose reference to the descriptor in real code without destroying the underlying packet.

I would suggest trying to fix this somehow such that the provided test case works; not by suggesting that users subclass from Application, because otherwise it is bound to reoccur since I have now seen three ns-3 users write code like this that avoids the use of base class Application.

The bottom line is that SOMEONE has to retain a reference to the socket throughout its lifetime.  So far we have two alternatives, one being that the node retains references to its sockets (Gustavo's suggestion), the other being that the user of sockets (Applications or end-users) keeps a reference (current expectation).

An approach we took in Tcp for a similar problem was simply to count the references to the socket that live in the Ipv4Endpoint demux:
http://code.nsnam.org/ns-3-dev/rev/4956586bd798

In UdpSocket::FinishBind, Note that "this" effectively copied into into the endpoint by passing the callback.  It is never Ref'd however.  Our fix was to pass a smart pointer into the callback, Ref'ing it in the process.

This way, when the endpoint is deallocated, the smart pointer goes out of scope and Unrefs the socket.  The assures that the lifetime of the socket is tied to the lifetime of its entry in the demux, which seems more correct to me.

So a change similar to what we did in TcpSocket should do the trick.

(In reply to comment #6)
> The bottom line is that SOMEONE has to retain a reference to the socket
> throughout its lifetime.  So far we have two alternatives, one being that the
> node retains references to its sockets (Gustavo's suggestion), the other being
> that the user of sockets (Applications or end-users) keeps a reference (current
> expectation).
> 
> An approach we took in Tcp for a similar problem was simply to count the
> references to the socket that live in the Ipv4Endpoint demux:
> http://code.nsnam.org/ns-3-dev/rev/4956586bd798
> 
> In UdpSocket::FinishBind, Note that "this" effectively copied into into the
> endpoint by passing the callback.  It is never Ref'd however.  Our fix was to
> pass a smart pointer into the callback, Ref'ing it in the process.
> 
> This way, when the endpoint is deallocated, the smart pointer goes out of scope
> and Unrefs the socket.  The assures that the lifetime of the socket is tied to
> the lifetime of its entry in the demux, which seems more correct to me.

What happens if a user releases the reference to the socket and he never calls Close ?

> 
> So a change similar to what we did in TcpSocket should do the trick.
> 

(In reply to comment #6)
> The bottom line is that SOMEONE has to retain a reference to the socket
> throughout its lifetime.  So far we have two alternatives, one being that the
> node retains references to its sockets (Gustavo's suggestion), the other being
> that the user of sockets (Applications or end-users) keeps a reference (current
> expectation).
> 
> An approach we took in Tcp for a similar problem was simply to count the
> references to the socket that live in the Ipv4Endpoint demux:
> http://code.nsnam.org/ns-3-dev/rev/4956586bd798
> 
> In UdpSocket::FinishBind, Note that "this" effectively copied into into the
> endpoint by passing the callback.  It is never Ref'd however.  Our fix was to
> pass a smart pointer into the callback, Ref'ing it in the process.
> 
> This way, when the endpoint is deallocated, the smart pointer goes out of scope
> and Unrefs the socket.  The assures that the lifetime of the socket is tied to
> the lifetime of its entry in the demux, which seems more correct to me.
> 
> So a change similar to what we did in TcpSocket should do the trick.

I was thinking along similar lines but, for UDP, the lifetime of the socket is much more problematic: i.e., it is not bound to an underlying stream which can be closed or not by the remote side. So, if the user forgets to call Close, the socket will live forever, even though it has data to send and cannot forward its incoming data to anyone.

I might be worried about a non-issue though.

(In reply to comment #5)
> I agree that it is not really intuitive the way the code now works, as
> exemplified by this test case.  I as a user would expect that if I used a
> socket and passed data to it, I could delete my local reference to the socket
> afterwards, just as I could lose reference to the descriptor in real code
> without destroying the underlying packet.

Sure, however, what is happening in the real world is that it is your application which keeps track of open file descriptors and the kernel frees them when the application dies. The problem here, is that you have associate your socket to a user context to be able to destroy it when the user context disapears and the user context in ns3 is the Application class.

(In reply to comment #7)
[snip]
> What happens if a user releases the reference to the socket and he never calls
> Close ?
> 
> I was thinking along similar lines but, for UDP, the lifetime of the socket is
> much more problematic: i.e., it is not bound to an underlying stream which can
> be closed or not by the remote side. So, if the user forgets to call Close, the
> socket will live forever, even though it has data to send and cannot forward
> its incoming data to anyone.
> 
> I might be worried about a non-issue though.

I think that the situation you are describing would become an error on the user's part under my proposal.  In the real world, if you don't call close on a socket, doesn't it persist indefinitely?  From this perspective, this is the correct behavior.

The Socket WILL continue to deliver data up to the application however via the callback mechanism.  The Socket persists and has a pointer to the application upcall.

And luckily for us, Simulator::Destroy will eventually lead to UdpL4Protocol::DoDispose, which will delete it's demux, the destructor of which will delete each endpoint, the destructor of which will release the last reference to the Socket (the ones that live in the endpoint destroy and rx callbacks).  So this is shouldn't leak memory from a valgrind perspective either.

(In reply to comment #9)
> (In reply to comment #7)
> [snip]
> > What happens if a user releases the reference to the socket and he never calls
> > Close ?
> > 
> > I was thinking along similar lines but, for UDP, the lifetime of the socket is
> > much more problematic: i.e., it is not bound to an underlying stream which can
> > be closed or not by the remote side. So, if the user forgets to call Close, the
> > socket will live forever, even though it has data to send and cannot forward
> > its incoming data to anyone.
> > 
> > I might be worried about a non-issue though.
> 
> I think that the situation you are describing would become an error on the
> user's part under my proposal.

Hey, your proposal is the same as mine :-)

I just did not understand the implementation very well...

>  In the real world, if you don't call close on a
> socket, doesn't it persist indefinitely?  From this perspective, this is the
> correct behavior.

Agreed, a socket should live until told to die.  A smart pointer going out of scope should not be enough to make a socket die.

> 
> The Socket WILL continue to deliver data up to the application however via the
> callback mechanism.  The Socket persists and has a pointer to the application
> upcall.
> 
> And luckily for us, Simulator::Destroy will eventually lead to
> UdpL4Protocol::DoDispose, which will delete it's demux, the destructor of which
> will delete each endpoint, the destructor of which will release the last
> reference to the Socket (the ones that live in the endpoint destroy and rx
> callbacks).  So this is shouldn't leak memory from a valgrind perspective
> either.

Indeed.

IMHO a socket, once created, should remain under custodian of its factory.  The factory should remove its reference to the socket when either:
 1. the factory dies (and the factory dies when the node is disposed, at the end of the simulation)
 2. it is explicitly closed, by calling Socket::Close ()
 3. the remote end point closes the connection, for connection oriented sockets.

I don't think this costs much in terms of memory (at most 16 bytes per socket; that's less than 1.6 MiB for 10^5 sockets).  And like Raj said, having to close() sockets is already what happens in the real world.

Created attachment 119 [details]
This is Gustavo's test case worked out in full.

(In reply to comment #11)
> Created an attachment (id=119) [edit]
> This is gustavo
> 

Should read "This is Gustavo's test case worked out in full"

Created attachment 120 [details]
Proposed solution

To clarify to Gustavo:

The implementation is as follows:  Instead of having the creator of the UdpSocket (what you call the factory) maintain a reference to it and consuming an extra amount of memory, this approach effectively Ref's the socket one more time to account for the pointer that exists to the socket in the endpoint callbacks (Ipv4EndPoint::m_rxCallback and m_destroyCallback).  From a reference counting perspective, I contend this is the right thing to do anyway.

This fix, while somewhat orthogonal to the issue at hand, also happens to solve the problem.

> IMHO a socket, once created, should remain under custodian of its factory.  The
> factory should remove its reference to the socket when either:
>  1. the factory dies (and the factory dies when the node is disposed, at the
> end of the simulation)
>  2. it is explicitly closed, by calling Socket::Close ()
>  3. the remote end point closes the connection, for connection oriented
> sockets.
> 
> I don't think this costs much in terms of memory (at most 16 bytes per socket;
> that's less than 1.6 MiB for 10^5 sockets).  And like Raj said, having to
> close() sockets is already what happens in the real world.
> 

Yes, but I think it is not completely clear cut.  We are debating the issue of what happens if the user forgets to close the socket.  In your proposal, we could leak sockets (descriptors) in such a case.  As presently implemented, we would delete the socket instead of closing it.  Neither really captures what happens in real code, in which close() is called when the process goes away before the user closed it.

Your and Raj's proposal would err on the side of keeping sockets around, and I would be fine with that, because I think it would be less intuitive to a user to have sockets disappear without even an implicit close() ever having occurred.  This is probably more relevant to consider with TCP but might be a problem with UDP if we ever model the case where the UDP socket didn't immediately send the datagram for some reason. 

there is the separate issue of whether ns-3 should do something when methods like Test::Run() are called before Simulation::Run() is called.  Clearly, it will provide undefined behavior to use parts of the API outside of a running simulation context.  Do we want to try to guard against this misuse somehow?

(In reply to comment #13)
> Created an attachment (id=120) [edit]
> Proposed solution
> 
> To clarify to Gustavo:
> 
> The implementation is as follows:  Instead of having the creator of the
> UdpSocket (what you call the factory) maintain a reference to it and consuming
> an extra amount of memory, this approach effectively Ref's the socket one more
> time to account for the pointer that exists to the socket in the endpoint
> callbacks (Ipv4EndPoint::m_rxCallback and m_destroyCallback).  From a reference
> counting perspective, I contend this is the right thing to do anyway.
> 
> This fix, while somewhat orthogonal to the issue at hand, also happens to solve
> the problem.

That sounds like a good solution as well, although it has slightly different semantics from what I proposed.  Under your solution, a reference is kept to the socket as long as there is a (rx,tx,destroy) callback associated with the socket.  That makes sense.  A socket with no callbacks and which goes out of scope should just be freed, as it will not prevent anything from working.

(In reply to comment #14)
> Yes, but I think it is not completely clear cut.  We are debating the issue of
> what happens if the user forgets to close the socket.  In your proposal, we
> could leak sockets (descriptors) in such a case.  As presently implemented, we
> would delete the socket instead of closing it.  Neither really captures what
> happens in real code, in which close() is called when the process goes away
> before the user closed it.

If you wanted to capture what _really_ happens in the real world, then sockets would always have to belong to applications, and be conceptually attached to applications:

  Ptr<Socket> SocketFactory::CreateSocket (Ptr<Application> application);

Since in the real world sockets are created by _processes_ making a system call into the kernel.  The kernel would implicitly receive the process ID as "parameter" and would associate the newly created socket with the process.

In NS-3, because users do not want to have to write new application classes (ugh!), we would need a 'generic' application.

Ptr<GenericApplication> Node::CreateApplication ();

Applications would then be release their own sockets when they are destroyed.

I think the above model would mimick best what happens in real world.  However, if applications are only destroyed when a node is destroyed, or when the programmer explicitly destroys them, their sockets will consequently also only be destroyed when the application is explicitly destroyed or when the node is destroyed.  And so this ultimately reallistic API turns is reduced to my proposed solution where sockets are destroyed explicitly or when the Node is destroyed.

I am in favour of making the Application "middle man" optional (personally I tend to never use it), and just allow sockets to be used directly.

> 
> Your and Raj's proposal would err on the side of keeping sockets around, and I
> would be fine with that, because I think it would be less intuitive to a user
> to have sockets disappear without even an implicit close() ever having
> occurred.  This is probably more relevant to consider with TCP but might be a
> problem with UDP if we ever model the case where the UDP socket didn't
> immediately send the datagram for some reason. 

What if I create a OnOff application and forget to dispose it?  It will keep on sending traffic forever...  same problem.

> 
> there is the separate issue of whether ns-3 should do something when methods
> like Test::Run() are called before Simulation::Run() is called.  Clearly, it
> will provide undefined behavior to use parts of the API outside of a running
> simulation context.  Do we want to try to guard against this misuse somehow?
> 

The logical solution would be:
1. Most APIs, like socket creation, fail an assertion when called and the simulator not running;
2. It would force programmers to do the simulation initialization stuff in a callback, scheduled with Simulator::ScheduleNow;

But I don't think that would help.  The Simulation initialization function would still create a local variable smart pointer, create the socket, and it would still go out of scope and destroy the socket before time.

I just wanted to say that today I found out the same socket error by the same student again, even after I explained the problem a few weeks ago.

But I give up trying to convince anyone anymore :-(

Raj, after today's conference call, you have the green light to commit your patch.

Is there anything blocking the patch?

(In reply to comment #19)
> Is there anything blocking the patch?

I will happily apply it if someone can confirm that we have no valgrind-reported memory leak regressions

Created attachment 138 [details]
raj's patch updated to ns-3-dev

Regarding valgrind, waf check --valgrind does not report any problem, with or without the patch.  waf regression --valgrind reports errors related to packet tags, with or without the patch.

(In reply to comment #21)

> Regarding valgrind, waf check --valgrind does not report any problem, with or
> without the patch.  waf regression --valgrind reports errors related to packet
> tags, with or without the patch.

Please, file a separate bug about the latter problem and merge this patch.

Filed bug #192 about the regressions.
Patch pushed.