Bugzilla – Bug 423
TCP copy constructor wrong
Last modified: 2008-12-11 23:46:43 EDT
You need to log in before you can comment on or make changes to this bug.
from a cursory code review of TcpSocketImpl::TcpSocketImpl (const TcpSocketImpl &o), it looks like there is a missing call to SetSendCallback (MakeNullCallback<void, Ptr<Socket>,uint32_t> () ); and probably of every other callback.
I thought we had agreed in the past that it is okay for the callbacks to be copied on a forked socket, except for the "Recv" callback? Can you provide some additional justification for why the callbacks shouldn't be copied?
This bug is on the ns-3.3 hot list and needs to be resolved one way or another.
(In reply to comment #1) > I thought we had agreed in the past that it is okay for the callbacks to be > copied on a forked socket, except for the "Recv" callback? Can you provide > some additional justification for why the callbacks shouldn't be copied? > I know we discussed that Recv should be nulled out (and it is) earlier this year. In returning to this, I'm wondering whether the default policy should be to null them all. For instance, should a single data sent callback be invoked whenever any individual socket sends data, or should the handler of the new connection register for data send callback with a new function? Is there a use case for not nulling them all?
(In reply to comment #1) > I thought we had agreed in the past that it is okay for the callbacks to be > copied on a forked socket, except for the "Recv" callback? Can you provide > some additional justification for why the callbacks shouldn't be copied? For example, the Send callbacks should not be copied. In my ns-3-simu code, I create a wrapper C++ object on top of each ns3::Socket instance to listen to all the socket events of interest and implement more fully the posix API. When I 'fork' a socket, that is, when I create a copy for a stream socket from the accept callback and store it in my queue of accepted sockets, I need to configure a different set of callbacks for this new socket to make sure that each new callback goes to my new C++ wrapper object associated to the new socket object. to summarize, in my code, I have a separate callback for each socket instance so, when I copy a socket, I have to provide a new set of callbacks and it makes no sense to reuse the previously-set callbacks. In that context, copying the callbacks is actively harmful because if I forget to reset one, I get event notifications on the wrong c++ wrapper. Not good and very hard to debug. So, I would support tom's suggestion to not copy any callback from the Socket base class copy constructor.
Created an attachment (id=334) [details] Null out the callbacks in the copy constructor. Fixes and passes regression and check tests.
changeset 4014:b6349d9c007e