changeset 2895	f8523d79a0b7 
fixes the specific problem of re-aggregating Internet stack twice.  (which shouldn't assert-- we should just avoid the re-aggregation)

But the general problem of detecting this isn't yet fixed.

(In reply to comment #1)
> changeset 2895  f8523d79a0b7 
> fixes the specific problem of re-aggregating Internet stack twice.  (which
> shouldn't assert-- we should just avoid the re-aggregation)

I think that this is wrong. It is a programming error to attempt to attach the same stack twice to a node. We need to assert. If there are helpers where we do this check already, then, these helpers should be changed to not contain the check.

> 
> But the general problem of detecting this isn't yet fixed.

It is trivial to iterate over the list of objects in Object::AggregateObject prior to adding an object to that list and assert there.

(In reply to comment #2)
> (In reply to comment #1)
> > changeset 2895  f8523d79a0b7 
> > fixes the specific problem of re-aggregating Internet stack twice.  (which
> > shouldn't assert-- we should just avoid the re-aggregation)
> 
> I think that this is wrong. It is a programming error to attempt to attach the
> same stack twice to a node. We need to assert. If there are helpers where we do
> this check already, then, these helpers should be changed to not contain the
> check.

The alternative is that we will not be able to pass in a container holding a multi-interface node to the internet stack helper.  See, for instance, the mixed-wireless script.

I don't think it is wrong to define the semantics of the AddInternetStack method to "add an IPv4 stack to the node if one has not already been added".  Can you elaborate why you think this is a bad idea?  

I should add that I went ahead and checked it in without posting a patch because I thought it would be non-controversial and that we had discussed this already in the past-- sorry.

Aha,

Would you object if I put this check here, instead of in AddInternetStack()?

void 
InternetStackHelper::Install (NodeContainer c)
{
  for (NodeContainer::Iterator i = c.Begin (); i != c.End (); ++i)
    {
      Ptr<Node> node = *i;
      AddInternetStack (node);
      Ptr<PacketSocketFactory> factory = CreateObject<PacketSocketFactory> ();
      node->AggregateObject (factory);
    }
}

> > > changeset 2895  f8523d79a0b7 
> > > fixes the specific problem of re-aggregating Internet stack twice.  (which
> > > shouldn't assert-- we should just avoid the re-aggregation)
> > 
> > I think that this is wrong. It is a programming error to attempt to attach the
> > same stack twice to a node. We need to assert. If there are helpers where we do
> > this check already, then, these helpers should be changed to not contain the
> > check.
> 
> The alternative is that we will not be able to pass in a container holding a
> multi-interface node to the internet stack helper.  See, for instance, the
> mixed-wireless script.

This just means that your script needs to be written in a slightly different
way: it needs to 

> 
> I don't think it is wrong to define the semantics of the AddInternetStack
> method to "add an IPv4 stack to the node if one has not already been added". 
> Can you elaborate why you think this is a bad idea?  

We already discussed that in the past in the case of the MobilityHelper::Layout
method: the issue is that if you allow what you suggest, you have to define
_exactly_ the semantics of doing this operation twice. For some helpers such as
the InternetStackHelper, it is not too hard and somewhat makes sense. For other
helpers, it is highly non-trivial (MobilityHelper) because it does interact in
very non-obvious ways with some of the features of said helper.

So, if you want to allow this feature in various helpers, then, you have to
document these sometimes non-trivial semantics on a per-helper basis which is
really a recipe for pain and disaster. It is vastly easier to document and
convey to users that each operation is allowed only once. Yes, it means that in
some cases, you will have to structure your program in a certain way and won't
be able to use some shortcuts. That "pain" is worth the overall conceptual
simplification I think.

(In reply to comment #4)
> I should add that I went ahead and checked it in without posting a patch
> because I thought it would be non-controversial and that we had discussed this
> already in the past-- sorry.

I think that it is okay to check in code which seems non-controversial (even if it is not in the end): it is easy to fix either through a trivial revert or another patch :)

(In reply to comment #5)

> Would you object if I put this check here, instead of in AddInternetStack()?

I object to the feature, not its implementation :)

(In reply to comment #6)
> It is vastly easier to document and
> convey to users that each operation is allowed only once. 

I do not understand this point of view, in this specific case.  First of all, Install() method is allowed to be invoked multiple times, if you consider other helpers.

Second, this is a helper API.  It would help the user to not have to create or maintain this special container that is only used to call InternetStackHelper::Install ().  Whether or not your suggestion to assert on finding a node with an internet stack already is adopted, users still need to understand that this Install () is different than the other (e.g., device) Install()s, semantically.  Why not make the semantics the most useful to the user?

I still favor 
  /**
   * \param c the set of nodes
   *
   * For each node in the input container, aggregate implementations
-   * of the ns3::Ipv4, ns3::Udp, and, ns3::Tcp classes.
+   * of the ns3::Ipv4, ns3::Udp, and, ns3::Tcp classes if they are 
+   * not already present in the node.
   */
  void Install (NodeContainer c);

(In reply to comment #9)
> (In reply to comment #6)
> > It is vastly easier to document and
> > convey to users that each operation is allowed only once. 
> 
> I do not understand this point of view, in this specific case.  First of all,
> Install() method is allowed to be invoked multiple times, if you consider other
> helpers.

Well, as I said in my first comment, if this is the case, then, it should be fixed.

> 
> Second, this is a helper API.  It would help the user to not have to create or
> maintain this special container that is only used to call
> InternetStackHelper::Install ().  Whether or not your suggestion to assert on
> finding a node with an internet stack already is adopted, users still need to
> understand that this Install () is different than the other (e.g., device)
> Install()s, semantically.  Why not make the semantics the most useful to the
> user?

Because that would make these semantics hard to document in a consistent way across multiple helpers. It is important to make the helper API easy to use _but_ that does not mean make it highly optimized to a specific use-case. It just means that it should not be too painful to use it and that a wide range of use-cases are covered and that the overall structure is consistent.

What you suggest is not adding functionality because you can already use the API to do what you want if you re-arange your code but it is adding extra complex logic in the Helper API. Ideally, that code would contain zero complex logic and be a trivial pass-thru because you want to make it easy to replace it with adhoc code when it does not do what you need.

This is really another instance of the Socket debate we had recently: you (users) just have to learn how the system is expected to work to use it efficiently and painlessly. It is not ok to add random extra hacks everywhere to attempt to save the user from having to learn how the system works because that is exactly the kind of process which lead to the current ns2 tcl code and you don't want to end up there.

> 
> I still favor 
>   /**
>    * \param c the set of nodes
>    *
>    * For each node in the input container, aggregate implementations
> -   * of the ns3::Ipv4, ns3::Udp, and, ns3::Tcp classes.
> +   * of the ns3::Ipv4, ns3::Udp, and, ns3::Tcp classes if they are 
> +   * not already present in the node.
>    */
>   void Install (NodeContainer c);

understood. Just don't do it :)

> 
> Because that would make these semantics hard to document in a consistent way
> across multiple helpers. It is important to make the helper API easy to use
> _but_ that does not mean make it highly optimized to a specific use-case. It
> just means that it should not be too painful to use it and that a wide range of
> use-cases are covered and that the overall structure is consistent.

We already have inconsistent API semantics here, even with your suggestion:

CsmaHelper::Install ();  // can be called repeatedly on the same container
InternetStackHelper::Install ();  // can not be called repeatedly on the same container
> 
> What you suggest is not adding functionality because you can already use the
> API to do what you want if you re-arange your code but it is adding extra
> complex logic in the Helper API. Ideally, that code would contain zero complex
> logic and be a trivial pass-thru because you want to make it easy to replace it
> with adhoc code when it does not do what you need.

what is complex about checking whether the node already has a stack?  The point of the helper API is to offload these details from the user.

> 
> This is really another instance of the Socket debate we had recently: you
> (users) just have to learn how the system is expected to work to use it
> efficiently and painlessly. It is not ok to add random extra hacks everywhere
> to attempt to save the user from having to learn how the system works because
> that is exactly the kind of process which lead to the current ns2 tcl code and
> you don't want to end up there.
>

I think that this frames the question the wrong way.  The question is not how users learn how the system works, but how do we want the system to work in the first place?  Remember, we are talking about helper API here.

If you really want consistent API, then you should not use Install() for InternetStackHelper, because it does not have the same semantics as any of the device Install()s.  Then, you can pick another name for InternetStackHelper::Install() (or DeviceHelper::Install()).  Once you have another name, you can define whatever semantics are most helpful for its usage.

(In reply to comment #11)
> > 
> > Because that would make these semantics hard to document in a consistent way
> > across multiple helpers. It is important to make the helper API easy to use
> > _but_ that does not mean make it highly optimized to a specific use-case. It
> > just means that it should not be too painful to use it and that a wide range of
> > use-cases are covered and that the overall structure is consistent.
> 
> We already have inconsistent API semantics here, even with your suggestion:
> 
> CsmaHelper::Install ();  // can be called repeatedly on the same container
> InternetStackHelper::Install ();  // can not be called repeatedly on the same
> container

I already wrote two times that if this is the case, then it should be fixed so, this is really a non-argument: I certainly never claimed that I had done a serious review of the helper API myself.


> > This is really another instance of the Socket debate we had recently: you
> > (users) just have to learn how the system is expected to work to use it
> > efficiently and painlessly. It is not ok to add random extra hacks everywhere
> > to attempt to save the user from having to learn how the system works because
> > that is exactly the kind of process which lead to the current ns2 tcl code and
> > you don't want to end up there.
> >
> 
> I think that this frames the question the wrong way.  The question is not how
> users learn how the system works, but how do we want the system to work in the
> first place?  Remember, we are talking about helper API here.

Yes, I agree but, I already explained why the semantics you describe are not desirable in the first place: there are _some_ helpers for which the behavior you describe will be very complex to document. More specifically, there is _one_ helper for which this is problematic: MobilityHelper::Layout uses the state from the reference point stack and it is very unclear how to document what happens when the reference point stack is non-empty and when Layout gets a called twice on an object which has already a mobility model attached to it. I have the gut feeling that doing what you suggest in this case would be a programming error so, I would not want this to be silently ignored.

Maybe the problem here is that MobilityHelper is trying to do too much (it certainly contains much more logic than I would want) so, maybe the solution would be to try to simplify it. If that could be done without loss of functionality, then I would ok with implementing what you suggest. 

> 
> If you really want consistent API, then you should not use Install() for
> InternetStackHelper, because it does not have the same semantics as any of the
> device Install()s.  Then, you can pick another name for
> InternetStackHelper::Install() (or DeviceHelper::Install()).  Once you have
> another name, you can define whatever semantics are most helpful for its usage.

Craig insisted a lot for the consistent name so, I would be very wary to go back to that decision and undo it. If we are serious about this consistency, then we also probably should review the existing code to see if we can reuse Install in OlsrHelper and MobilityHelper.

There are two issues here.  The first is the actual bug.  It *is* a programming error to attempt to aggregate more than one "interface" of a given type.  That should assert, which will address bug 162.

The second issue is what semantics of Install in the helpers are.  I tend to look at the building of topologies like people building networks in reality.  If I were to give you a phone call and say, "please install the ns3 protocol stack on computer X," you'd go to that computer and find a stack already there.  The question is what do you do now?  Do you get excited and call back, saying "it's a disaster, there'e already one there" (ERROR) or do you call back saying, "it's okay, someone else got to it first so I just left it alone" (NOERROR).

It seems quite simple at first, but as Mathieu is trying to explain, it can get tricky.  In the simple case (one kind of internet stack), the answer is simple and you can make the case that the NOERROR approach from above works fine.  What if there are two internet stacks in play in the system of the same type (one *replacing* the other)?  What if you want to install stack of kind 2 on a node that already has kind 1 aggregated.  In this case, Install of stack 2 would silently pass and you'd have a system that was subtly and quietly different than you expected.  This is very bad.

Having semantics where Install says, "please ensure that this action is done exactly once" is easy and makes sense in the simple case; but it can cause horrible problems if you try to do perhaps unexpected things.

The same set of arguments actually applies to all of the helpers.  In previous versions of the helpers, I was playing with the concept of an "external" node to prevent this from happening.  If the "external" bit was set, I assumed that features were previously added by a set of helpers external to the currently running set.  The "external" bit was set when you Add()ed a node to a container.

In the current model of how the helpers are used, this is really no longer possible.  In many of the examples, you create N nodes in a container and then add them to various other containers which are eventually used to Install() the other pieces -- often *all* of the nodes are external in the sense I used them previously.  Basically anything can come from anywhere with no restrictions or assumptions, which is what Mathieu wanted, I suspect.

Given the current helper architecture, and the bad things that can possibly happen, I think Mathieu is on the right track here.  Just pass through the request to aggregate a second time and assert.  If you assert, fix your script so you don't assert.

I think discussions are diverging a bit from the main topic.

Adding the same interface to and object twice is a programming error and there should be an assertion failure.  I believe in this very strongly, it is the most important issue, and amazingly is the one issue not yet fixed!  Does anyone agree on this point at least?

After this issue is fixed, discussions can continue regarding helpers, perhaps in another bug report.  But the main issue is with the Object class, _not_ helpers.

(In reply to comment #14)
> I think discussions are diverging a bit from the main topic.

Well, it is part of the bug topic because tom pushed a fix to workaround the bug you describe below. So, the workaround itself became a legitimate issue to discuss here :)

> 
> Adding the same interface to and object twice is a programming error and there
> should be an assertion failure.  I believe in this very strongly, it is the
> most important issue, and amazingly is the one issue not yet fixed!  Does
> anyone agree on this point at least?

I think that this is non-controversial and is easy to fix. I am just busy with some other crap right now.

It doesn't seem worth a patch ...

I can push the following addition to Object::AggregateObject() if it's aggreable.

  if (DoGetObject (o->m_tid))
    {
      NS_FATAL_ERROR ("Object::AggregateObject(): "
       "Multiple aggregation of objects of type " << o->m_tid.GetName ());
    }

Regression tests are good :-)

Both the csma-broadcast and the csma-multicast examples error out with the multiple aggregation check present.

Object::AggregateObject(): Multiple aggregation of objects of type ns3::PacketSo
cketFactory

I'll have to fix those as well.

((In reply to comment #13)
> There are two issues here.  The first is the actual bug.  It *is* a programming
> error to attempt to aggregate more than one "interface" of a given type.  That
> should assert, which will address bug 162.
> 
> The second issue is what semantics of Install in the helpers are.  I tend to
> look at the building of topologies like people building networks in reality. 
> If I were to give you a phone call and say, "please install the ns3 protocol
> stack on computer X," you'd go to that computer and find a stack already there.
>  The question is what do you do now?  Do you get excited and call back, saying
> "it's a disaster, there'e already one there" (ERROR) or do you call back
> saying, "it's okay, someone else got to it first so I just left it alone"
> (NOERROR).

It is a bit more complicated than handling one node.  To use your analogy, what
we are saying is that you've been given the task "Please install the stack on
all of the computers in this topology."  You could report back i) "I found that
X out of Y already had the stack, so I skipped those, but I did install it on
the other nodes, so now every node in the container has exactly one Internet
stack."  or you could report back "I found at least one node in the container 
that already had a stack.  Please figure out which nodes already have stacks
and
eliminate those from the container, and give me a revised container, and I will 
add the stack to them if you've done your job correctly."

I am saying that the first case seems more "helpful" without being semantically
vague.

> 
> Having semantics where Install says, "please ensure that this action is done
> exactly once" is easy and makes sense in the simple case; but it can cause
> horrible problems if you try to do perhaps unexpected things.
> 
> The same set of arguments actually applies to all of the helpers.  In previous
> versions of the helpers, I was playing with the concept of an "external" node
> to prevent this from happening.  If the "external" bit was set, I assumed that
> features were previously added by a set of helpers external to the currently
> running set.  The "external" bit was set when you Add()ed a node to a
> container.
> 
> In the current model of how the helpers are used, this is really no longer
> possible.  In many of the examples, you create N nodes in a container and then
> add them to various other containers which are eventually used to Install() the
> other pieces -- often *all* of the nodes are external in the sense I used them
> previously.  Basically anything can come from anywhere with no restrictions or
> assumptions, which is what Mathieu wanted, I suspect.

This seems better to me, since similarly, tracking "external" vs. "internal"
seems cumbersome from a user perspective.

> 
> Given the current helper architecture, and the bad things that can possibly
> happen, I think Mathieu is on the right track here.  Just pass through the
> request to aggregate a second time and assert.  If you assert, fix your script
> so you don't assert.
> 

hmm, still not convinced.  We made a decision to delegate responsibility for
correct programming to the client of the low-level API; that I agree with. 
However, the helper API (a client of the low-level API) is supposed to simplify 
the user code, and IMO should be permitted to do some sanity checking of user 
input so that users don't have to do it themselves.

I guess I just don't see the danger of this instance, where I see the
cumbersome artifacts of having to track these container members in the 
complicated topologies.

If we agree that Install should have the same semantics "Add exactly one more,
no matter what", then I guess I am suggesting that we should invent a different
method name for InternetStackHelper::Install() and have its semantics be "Add
one if none exists."

Okay, I get to eat my own dogfood.  The problem in the csma code follows,

  NodeContainer c;
  c.Create (3);
  NodeContainer c0 = NodeContainer (c.Get (0), c.Get (1));
  NodeContainer c1 = NodeContainer (c.Get (0), c.Get (2));
  ...
  InternetStackHelper internet;
  internet.Install (c0);
  internet.Install (c1);

The zeroeth node created in container "c" gets internet.Install()ed twice. 
Tom's check detects for duplication of the stack itself, but misses the packet
socket factory aggregation in the helper itself.

I'll start fixing all of this if we agree that the "assert and fix" approach
advocated by Mathieu and (sigh)me is the way to go.

I looked, and fixing the example scripts to avoid asserting is trivial.

  InternetStackHelper internet;
  internet.Install (c);

instead of

  InternetStackHelper internet;
  internet.Install (c0);
  internet.Install (c1);

I can check in a fix for bug 162 any time.

Addressing Tom's comment #18

> This seems better to me, since similarly, tracking "external" vs. 
> "internal" seems cumbersome from a user perspective.

Not that it matters anymore, but the user didn't track it; the helper did.

> I am saying that the first case seems more "helpful" without being semantically

It is certainly more "helpful" and no one contested that (although I would
identify it as only very trivially and marginaly helpful), but, you seem to be
ignoring what I repeat in pretty much every post, that is, [...]

> vague.

[...] it _is_ semantically vague and hard to describe for _some_ helpers. So,
if you cannot make it crystal-clear for all helpers, you should not do it at
all to ensure a consistent behavior across all helpers. Yes, this is akin to
throwing the baby with the bath's water, but I don't know of any other good
solution to ensure what we promissed to craig, that is, consistent behaviors
across all helpers.

> hmm, still not convinced.  We made a decision to delegate responsibility for
> correct programming to the client of the low-level API; that I agree with. 
> However, the helper API (a client of the low-level API) is supposed to simplify 
> the user code, and IMO should be permitted to do some sanity checking of user 
> input so that users don't have to do it themselves.
> 
> I guess I just don't see the danger of this instance, where I see the
> cumbersome artifacts of having to track these container members in the 
> complicated topologies.

It would be easier to convince us that this is important if you could show
scripts for which the alternative will be painful. I have a hard time seeing
how you could construct an example which cannot be trivially updated to work
with the proposed semantics.

> 
> If we agree that Install should have the same semantics "Add exactly one more,
> no matter what", then I guess I am suggesting that we should invent a different
> method name for InternetStackHelper::Install() and have its semantics be "Add
> one if none exists."

I really don't see which use-case such an extra function would cater for beyond
just lazyness. Are there scripts which cannot be written without much pain with
the proposed restrictive semantics ? Please, prove me wrong.

I really hate having extended discussions here on bugzilla.  Can I just close out this bug by pushing the changes I have and then we can have a debate on what to do about Install() semantics on the list?  Please.

(In reply to comment #20)
> I looked, and fixing the example scripts to avoid asserting is trivial.
> 
>   InternetStackHelper internet;
>   internet.Install (c);
> 
> instead of
> 
>   InternetStackHelper internet;
>   internet.Install (c0);
>   internet.Install (c1);

yes, but it is not as simple in mixed-wireless.  The issue is that you need to add the internet stack before you assign addresses, so when you are in the middle of the for loop, you need two containers.

I seem to have no support for my objections so I will agree to drop it and support Mathieu's suggestion, so we can move forward. 

Shouldn't mixed-wireless be in ns-3-dev/examples?  

I took Tom's comment #23 as tacit approval for checking in my fix and closing the bug.