geodynamics / aspect

A parallel, extensible finite element code to simulate convection in both 2D and 3D models.
https://aspect.geodynamics.org/
Other
223 stars 236 forks source link

Initialization order is error-prone. #4906

Open bangerth opened 2 years ago

bangerth commented 2 years ago

In working on #4889, I realized something that had previously occurred to me: When we create the various plugin systems in the constructor of Simulator, we create some of the variables in the member variable initializer list and others in the body of the constructor. For the latter, we generally follow logic such as

    for (const auto &p : parameters.prescribed_traction_boundary_indicators)
      {
        BoundaryTraction::Interface<dim> *bv
          = BoundaryTraction::create_boundary_traction<dim>
            (p.second.second);
        boundary_traction[p.first].reset (bv);
        if (SimulatorAccess<dim> *sim = dynamic_cast<SimulatorAccess<dim>*>(bv))
          sim->initialize_simulator(*this);
        bv->parse_parameters (prm);
        bv->initialize ();
      }

This is awkward (and cost me ~2 hours of debugging in #4891) because during initialization of some objects, we are accessing other objects via the SimulatorAccess class. A better design ensures that we first create all objects in a first phase, and then initialize all in a second phase. I believe that we should split that second phase into calling parse_parameters() for all objects first, and then call initialize() later -- so, three phases.

tjhei commented 2 years ago

Makes sense to me.

gassmoeller commented 2 years ago

I think this is reasonable. One thing you need to be careful with is that the order of the plugin systems remains exactly the same. The current order is the result of a lot of reshuffling until all dependencies are satisfied. I hope none of the plugins require initialized information inside parse_parameters (but if they do the tests hopefully catch it, and that should be fixed anyway).