Bug #3318
closedMissing beforeChangePersistency override in some Transport subclasses
100%
Description
Not all Transport
subclasses currently available in NFD implement an overridden beforeChangePersistency
method, although they all should. Specifically:
MulticastUdpTransport
only supports permanentUnicastUdpTransport
supports all 3 kinds of persistency, but not all transitions are allowed (this logic is currently inUdpChannel
but it should be removed from there)UnixStreamTransport
only supports on-demandTcpTransport
currently implements an override which accepts the transition on-demand <-> persistent in either direction, I'm not sure if this is correct (what's the difference semantically between on-demand and persistent in the TCP case anyway?)InternalForwarderTransport
only supports permanent
EthernetTransport
and WebSocketTransport
already implement a correct override as far as I can see.
I also propose to make Transport::beforeChangePersistency
pure virtual. The reason is that the great majority of transports will have some kind of restriction in their persistency setting, therefore a pure virtual method ensures that subclasses won't forget to override it.
Updated by Junxiao Shi about 9 years ago
- Description updated (diff)
UnicastUdpTransport
supports all 3 kinds of persistency, but not all transitions are allowed (this logic is currently inUdpChannel
but it should be removed from there)
TcpTransport
currently implements an override which accepts the transition on-demand <-> persistent in either direction, I'm not sure if this is correct
All transitions should be allowed at NFD side, as long as destination persistency level is supported. See #3232 note-1.
The current "upgrade only" restriction is a workaround for the lack of faces/update
command: we don't want a faces/create
command to decrease the persistency level.
what's the difference semantically between on-demand and persistent in the TCP case anyway?
No difference at transport level.
There will be differences in BFD and tunnel authentication, but they are not at transport level.
But I don't want to modify this for now.
make
Transport::beforeChangePersistency
pure virtual
Agreed.
Updated by Davide Pesavento about 9 years ago
Junxiao Shi wrote:
All transitions should be allowed at NFD side, as long as destination persistency level is supported. See #3232 note-1.
The current "upgrade only" restriction is a workaround for the lack offaces/update
command: we don't want afaces/create
command to decrease the persistency level.
Fine, I don't disagree with that, but it doesn't change the fact that it's currently not allowed to follow the permanent -> persistent -> on-demand direction. Or at least UdpChannel
doesn't allow it at the moment. I don't want to change the current behavior, just to consolidate the validation in one place (beforeChangePersistency
). Restrictions can always be lifted at a later time. I didn't understand if you agree with that or not.
what's the difference semantically between on-demand and persistent in the TCP case anyway?
No difference at transport level.
There will be differences in BFD and tunnel authentication, but they are not at transport level.
But I don't want to modify this for now.
So, should on-demand <-> persistent transitions be allowed or not in TcpTransport
?
Updated by Junxiao Shi about 9 years ago
I don't want to change the current behavior, just to consolidate the validation in one place (
beforeChangePersistency
).
No. This would be an unnecessary change because #3232 is going to lift the restriction. For now, keep the restriction in UdpChannel
unchanged.
So, should on-demand <-> persistent transitions be allowed or not in
TcpTransport
?
Keep it unchanged.
Updated by Davide Pesavento about 9 years ago
- Status changed from New to Code review
- % Done changed from 0 to 100
Updated by Junxiao Shi about 9 years ago
make
Transport::beforeChangePersistency
pure virtual
#3259 note-1 says:
virtual calls from constructors become non-virtual, so
setPersistency
would end up calling the base class version ofbeforeChangePersistency
, which does nothing.
If we make beforeChangePersistency
pure-virtual, and a transport constructor calls setPersistency
which in turn calls beforeChangePersistency
, doesn't this call a pure-virtual function and cause a crash?
NFD:commit:40399e3f3fb0c93c063e0331553f7d930e559ce6 has this beforeChangePersistency
pure-virtual implement and build is passing, but I want to know why.
Updated by Davide Pesavento about 9 years ago
Uhm this is very interesting. I have no idea why it works... it should not.
My theory is that the compiler is able to speculatively devirtualize the invocation of beforeChangePersistency
, turning it into a check + direct (non-virtual) call to the derived class method. This is probably feasible because setPersistency
is inlined.
However, we cannot rely on the behavior resulting from this optimization. Virtual calls in constructors are a bad idea.
Updated by Junxiao Shi about 9 years ago
From MSDN C++ Constructors:
We recommend that you be careful when you call virtual functions in constructors. Because the base class constructor is always invoked before the derived class constructor, the function that's called in the base constructor is the base class version, not the derived class version.
If Transport
constructor calls setPersistency
, it would invoke Transport::beforeChangePersistency
pure virtual method and cause undefined behavior.
When TcpTransport
constructor calls setPersistency
, it's calling TcpTransport::beforeChangePersistency
which is valid.
#3259 note-1 incorrectly states that TcpTransport
constructor would call Transport::beforeChangePersistency
.
However, a better solution is: in Transport::setPersistency
, allow transitioning from NONE to any other persistency level without calling beforeChangePersistency
, and forbid transitioning into NONE.
This works because NONE is an invalid value so there's no point transitioning into it, and the transition from NONE must be in the subclass constructor (guaranteed by StaticProperties
test case) which can check the new persistency level by itself.
Note that implementing this change requires moving Transport::setPersistency
out of inline header.
Updated by Davide Pesavento about 9 years ago
Junxiao Shi wrote:
From MSDN C++ Constructors:
We recommend that you be careful when you call virtual functions in constructors. Because the base class constructor is always invoked before the derived class constructor, the function that's called in the base constructor is the base class version, not the derived class version.
This doesn't say anything regarding our scenario, i.e. a derived class constructor indirectly calling a virtual function.
If
Transport
constructor callssetPersistency
, it would invokeTransport::beforeChangePersistency
pure virtual method and cause undefined behavior.
WhenTcpTransport
constructor callssetPersistency
, it's callingTcpTransport::beforeChangePersistency
which is valid.
#3259 note-1 incorrectly states thatTcpTransport
constructor would callTransport::beforeChangePersistency
.
The above differs from what I've read online (and from what I remember from my C++ programming class), i.e. that the concept of "dynamic type" is completely ignored in constructors. However all examples I've seen refer to a base class constructor invoking the virtual function, and our case is a little different.
So at this point I was confused and I decided to look at the C++ standard. Paragraph 12.7.4 says:
When a virtual function is called directly or indirectly from a constructor or from a destructor, including
during the construction or destruction of the class’s non-static data members, and the object to which the
call applies is the object (call it x) under construction or destruction, the function called is the final overrider
in the constructor’s or destructor’s class and not one overriding it in a more-derived class.
It's quite clear that to determine what function is called you have to look at the type of the (sub-)object under construction, i.e. the type whose constructor is currently executing, NOT at the static type of this
at the virtual function call site (Transport::setPersistency
in our case). Therefore there is some kind of virtual dispatch, but it's limited to the current class, other "more-derived" classes are ignored.
However, a better solution is: in
Transport::setPersistency
, allow transitioning from NONE to any other persistency level without callingbeforeChangePersistency
, and forbid transitioning into NONE.
Yes, this is better than leaving the virtual call from the constructor, everywhere I looked said it's considered bad design.
Updated by Junxiao Shi about 9 years ago
- Status changed from Code review to Closed