Bug 1644

Summary: memory disposal in flow monitor causes valgrind error
Product: ns-3 Reporter: Tom Henderson <tomh>
Component: flow-monitorAssignee: Tom Henderson <tomh>
Status: RESOLVED FIXED    
Severity: normal CC: ns-bugs
Priority: P4    
Version: pre-release   
Hardware: All   
OS: All   

Description Tom Henderson 2013-04-25 09:59:49 UTC 
running valgrind on the newly added examples/tcp/tcp-variants-comparison.cc example leads to errors symptomatic of reference cycles.

I looked at this briefly and there is no use presently of Object::DoDispose in these objects to try to break these cycles, so my guess is that adding these kind of disposal methods may fix this.
Comment 1 Gustavo J. A. M. Carneiro 2013-04-25 10:12:01 UTC 
There are not actual pointers to anything that could cause a cycle, in flow monitor, at least no explicitly, though there are implicit pointers being created due to TraceConnect in Ipv4FlowProbe:

  if (!ipv4->TraceConnectWithoutContext ("UnicastForward",
                                         MakeCallback (&Ipv4FlowProbe::ForwardLogger, Ptr<Ipv4FlowProbe> (this))))

This creates a pointer to this Ipv4FlowProbe instance, and implicitly stores this pointer in the Ipv4 instance.

So, yes, there is a cycle.  We would need to somehow "disconnect" all those trace sources in DoDispose to break the cycle, I think.
Comment 2 Tom Henderson 2013-04-26 10:07:12 UTC 
I looked at a few examples; the destructors of FlowMonitor and the probes are never being called, nor is the DoDispose() automatically called (if such method is added).  We haven't been valgrind checking these examples until the tcp-variants-comparison.cc was added, but it seems that it just uncovered the problem.

The flow monitor and probes exist outside the context of the nodes/channels, only bound to them via trace sources.  So calls to destroy the nodes/channels and all those objects aggregated to them will not get to the flow monitor objects.

What do you think about making the FlowMonitorHelper clean up after its allocations when its destructor is called, since it is the one allocating and binding things together?  It seems to me that it would just mean that we disallow this usage pattern:

  // Flow monitor
  Ptr<FlowMonitor> flowMonitor;
  if (flow_monitor)
    {
      FlowMonitorHelper flowHelper;
      flowMonitor = flowHelper.InstallAll();
    }

If you agree with the idea, I can try to patch this later today.
Comment 3 Gustavo J. A. M. Carneiro 2013-04-26 10:39:18 UTC 
That makes sense.
You just need to track the main Ptr<FlowMonitor>, Dispose() it, and the FlowMonitor should in turn Dipose() all the probes and classifiers.  Finally, some code needs to be added to Ipv4FlowProbe to disconnect the trace sources.
Comment 4 Tom Henderson 2013-10-11 13:34:40 UTC 
was fixed in changeset cb763839fc18