Project

General

Profile

Actions

Bug #3318

closed

Missing beforeChangePersistency override in some Transport subclasses

Added by Davide Pesavento about 9 years ago. Updated almost 9 years ago.

Status:
Closed
Priority:
Normal
Category:
Faces
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:

Description

Not all Transport subclasses currently available in NFD implement an overridden beforeChangePersistency method, although they all should. Specifically:

  • MulticastUdpTransport only supports permanent
  • UnicastUdpTransport supports all 3 kinds of persistency, but not all transitions are allowed (this logic is currently in UdpChannel but it should be removed from there)
  • UnixStreamTransport only supports on-demand
  • TcpTransport 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.

Actions #1

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

Actions #2

Updated by Davide Pesavento almost 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 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?

Actions #3

Updated by Junxiao Shi almost 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.

Actions #4

Updated by Davide Pesavento almost 9 years ago

  • Description updated (diff)
Actions #5

Updated by Davide Pesavento almost 9 years ago

  • Status changed from New to Code review
  • % Done changed from 0 to 100
Actions #6

Updated by Junxiao Shi almost 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 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.

Actions #7

Updated by Davide Pesavento almost 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.

Actions #8

Updated by Junxiao Shi almost 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.

Actions #9

Updated by Davide Pesavento almost 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 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.

Actions #10

Updated by Junxiao Shi almost 9 years ago

  • Status changed from Code review to Closed
Actions

Also available in: Atom PDF