NFD

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 of faces/update command: we don't want a faces/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?

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.

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 of beforeChangePersistency, 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.

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.

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.

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 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.

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 calling beforeChangePersistency, 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.