RemoveVariable() doesn't fully remove Variable

[germasch]

This is going to be a bit convoluted. So looking at the core::IO code, thought I saw a bug in how the variable maps are handled. So I tried to actually trigger it, but I couldn't (immediately).

Instead, I found another bug: When the Variable is removed from the variableMap, the code actually gets a copy of the entire map, then removes the Variable from the copy, and the original map is unchanged. So that's easy to fix:

--- a/source/adios2/core/IO.cpp
+++ b/source/adios2/core/IO.cpp
@@ -165,7 +165,7 @@ bool IO::RemoveVariable(const std::string &name) noexcept
 #define declare_type(T)                                                        \
     else if (type == helper::GetType<T>())                                     \
     {                                                                          \
-        auto variableMap = GetVariableMap<T>();                                \
+        auto& variableMap = GetVariableMap<T>();                                \
         variableMap.erase(index);                                              \
         isRemoved = true;                                                      \
     }

Now, however, a more fundamental problem with these maps appears, that is, they get into inconsistent state when a variable is removed and then another variable is added. Here is a test:

void test2()
{
  using T = int32_t;

  adios2::ADIOS ad("adios.xml", MPI_COMM_WORLD, adios2::DebugON);

  {
    adios2::IO io_writer = ad.DeclareIO("io_writer");
    auto var = io_writer.DefineVariable<T>("var");
    auto var2 = io_writer.DefineVariable<T>("var2");
    auto writer = io_writer.Open("test", adios2::Mode::Write);
    io_writer.RemoveVariable("var");
    auto var3 = io_writer.DefineVariable<T>("var3");
    std::cout << "var2 name " << var2.Name() << std::endl;
    std::cout << "var3 name " << var3.Name() << std::endl;
    
    T test = 99;
    writer.Put(var3, test);
    writer.Close();
  }
}

When run, it prints

var2 name var2
var3 name var2

var3 didn't really get added, instead it became an alias for var2. The problem can be more explicitly seen with this patch:

diff --git a/source/adios2/core/IO.tcc b/source/adios2/core/IO.tcc
index f595748e..447d03f7 100644
--- a/source/adios2/core/IO.tcc
+++ b/source/adios2/core/IO.tcc
@@ -48,6 +48,8 @@ Variable<T> &IO::DefineVariable(const std::string &name, const Dims &shape,
     auto itVariablePair =
         variableMap.emplace(size, Variable<T>(name, shape, start, count,
                                               constantDims, m_DebugMode));
+    bool emplaceSucceeded = itVariablePair.second;
+    if (!emplaceSucceeded) throw std::runtime_error("emplace failed in IO::DefineVariable");
     m_Variables.emplace(name, std::make_pair(helper::GetType<T>(), size));

     Variable<T> &variable = itVariablePair.first->second;

which will make it crash rather than exhibit the incorrect behavior. (Note: There may be a point in always checking the return value from std::map::emplace(), at least in an assert() kind of fashion).

The underlying problem is the one I noted when reading DefineVariable in the first place: The code is assuming that variableMap.size() is going to give it a new, unused index, but that is not the case with a map that can shrink. The easiest solution I can think of is to just maintain a per-map index which gets incremented for each Variable that's inserted, but never decremented. That should be fine as long as it doesn't wrap around.

[williamfgc]

@germasch yes, this is definitively a bug on my side, thanks! These remove functions are part of the danger zone and were added much later for very corner cases and not throughly tested. We added RemoveIO which is more general.