Fix envoy crash during early shutdown

[eziskind]

Description: This fixes a crash during early shutdown that occurs in the InstanceImpl destructor when it frees the ListenerManagerImpl object. PR #4244 tried to fix this but we're still seeing this periodically. In the crash scenario, the RdsRouteConfigSubscription destructor calls the initialize_callback_ function which ends up calling the InitManager initialize lambda created in the RunHelper constructor. That lambda has a call to workers_start_cb which is protected by a check on the RunHelpers shutdown_ field. However at the point this lambda is executed the RunHelper object has already been destructed (in InstanceImpl::run) so the shutdown_ field is dangling and can contain arbitrary content. In this case we ended up falling through and calling the workers_start_cb function which called InstanceImpl::startWorkers which caused the crash because InstanceImpl::listener_manager_ was null. This fix avoids the crash by making the lambda passed to the InitManager::initialize method not bind to the RunHelper object since it may not exist at the time the lambda is executed. Instead keep the shutdown state in the InstanceImpl object itself.
Risk Level: low
Testing: unit testing

[htuch]

Do you know if the "don't call virtual functions in destructor" (e.g. https://www.artima.com/cppsource/nevercall.html) rule applies here?

[eziskind]

Thanks for the pointer. I think the way I'm using it here is safe though - here is the relevant bit "Once a derived class destructor has run, the object's derived class data members assume undefined values, so C++ treats them as if they no longer exist. Upon entry to the base class destructor, the object becomes a base class object".

In this case, we're calling the virtual method (isShutdown) from the derived class destructor (InstanceImpl) not that of the base class (Instance) so the object should still be treated as an InstanceImpl and call the correct method.

[lizan]

@htuch Actually clang-analyzer-optin.cplusplus.VirtualCall check will warn about this (though, not an error now so it will pass) in clang-tidy CI. It doesn't seems the case?

In this case, we're calling the virtual method (isShutdown) from the derived class destructor (InstanceImpl) not that of the base class (Instance) so the object should still be treated as an InstanceImpl and call the correct method.

@eziskind if you make the virtual method called by the derived class destructor final, then clang-analyzer won't complain.

[eziskind]

done

[mattklein123]

Makes sense to me. Thanks for fixing!

[htuch]

Thanks!