Tweak Remote class and test multi-threaded file remote access

[eisenhauer]

No description provided.

[anagainaru]

I read about Singleton yesterday to make sure I understand what you are doing here and this code is correct, but I have a few comments that I want to check with you.

1. The only reason you need the mutex is because you need the first variable to be true only for the thread that creates the singleton. The recommendation is to create the singleton in a lambda function that also does all the operations that need to be executed only once. The lambda is guaranteed to be called by a single thread. So, my first question is, could you do all the RegisterFormats and CMregister_handler calls in the Singleton? By looking at the code I would say you can. If yes we don't need the mutex.
2. There is a small memory leak caused by new. Unless there is a reason to return a pointer I would return the singleton by reference.

I wrote a bunch of code to try to understand how the singleton works so if you agree with the two points I can send a draft commit with what I'm thinking and you can modify it or trash it. This being said your code does the job, I just think it might be cleaner with the changes based of what I've read.

[anagainaru]

These are the changes I'm proposing:

Author: anagainaru <ana.gainaru@gmail.com>
Date:   Sat Oct 7 13:17:28 2023 -0400

    Clean the singleton code

diff --git a/source/adios2/toolkit/remote/Remote.cpp b/source/adios2/toolkit/remote/Remote.cpp
index 0fe319942..2adb8bb80 100644
--- a/source/adios2/toolkit/remote/Remote.cpp
+++ b/source/adios2/toolkit/remote/Remote.cpp
@@ -50,15 +50,12 @@ void ReadResponseHandler(CManager cm, CMConnection conn, void *vevent, void *cli
     return;
 };

-void Remote::InitCMData()
+CManagerSingleton& CManagerSingleton::Instance(RemoteCommon::Remote_evpath_state &ev_state)
 {
-    std::lock_guard<std::mutex> lockGuard(m_CMInitMutex);
-    bool first = true;
-    auto CM = CManagerSingleton::Instance(first);
-    ev_state.cm = CM->m_cm;
-    RegisterFormats(ev_state);
-    if (first)
-    {
+    static CManagerSingleton self = [&ev_state] {
+        CManagerSingleton instance;
+        ev_state.cm = instance.m_cm;
+        RegisterFormats(ev_state);
         CMfork_comm_thread(ev_state.cm);
         CMregister_handler(ev_state.OpenResponseFormat, (CMHandlerFunc)OpenResponseHandler,
                            &ev_state);
@@ -68,7 +65,14 @@ void Remote::InitCMData()
                            (CMHandlerFunc)OpenSimpleResponseHandler, &ev_state);
         CMregister_handler(ev_state.ReadResponseFormat, (CMHandlerFunc)ReadResponseHandler,
                            &ev_state);
-    }
+        return instance;
+    } ();
+    return self;
+}
+
+void Remote::InitCMData()
+{
+    (void) CManagerSingleton::Instance(ev_state);
 }

 void Remote::Open(const std::string hostname, const int32_t port, const std::string filename,
diff --git a/source/adios2/toolkit/remote/Remote.h b/source/adios2/toolkit/remote/Remote.h
index 0e39d8964..aaa1b89e9 100644
--- a/source/adios2/toolkit/remote/Remote.h
+++ b/source/adios2/toolkit/remote/Remote.h
@@ -63,34 +63,20 @@ private:
     bool m_Active = false;
 };

+#ifdef ADIOS2_HAVE_SST
 class CManagerSingleton
 {
 public:
-#ifdef ADIOS2_HAVE_SST
+    static CManagerSingleton& Instance(RemoteCommon::Remote_evpath_state &ev_state);
+
+private:
     CManager m_cm = NULL;
-#endif
-    static CManagerSingleton *Instance(bool &first)
-    {
-        static std::mutex init_mutex;
-        const std::lock_guard<std::mutex> lock(init_mutex);
-        static CManagerSingleton *ptr = new CManagerSingleton();
-        static bool internal_first = true;
-        first = internal_first;
-        internal_first = false;
-        return ptr;
-    }
-
-protected:
-#ifdef ADIOS2_HAVE_SST
+
     CManagerSingleton() { m_cm = CManager_create(); }

     ~CManagerSingleton() { CManager_close(m_cm); }
-#else
-    CManagerSingleton() {}
-
-    ~CManagerSingleton() {}
-#endif
 };
+#endif

 } // end namespace adios2

[eisenhauer]

Hmm. I tend to agree with your comments. I had been following my original thinking, where my focus was on making sure we shared one CM per process, so I put it into the singleton. That was fine with the Get() level stuff, but the transports being threaded wasn't something I had thought about in advance and I ran into data races. Returning the first flag was my reaction, but probably not best. Let me re-evaluate Monday...

[anagainaru]

I went down a C++ rabbit hole and I have a better idea. There is std::call_once where we can do all of CManager create and hander register calls, and we call in it std::atexit where we call CManager_close. We can discuss tomorrow.

[eisenhauer]

I went down a C++ rabbit hole and I have a better idea. There is std::call_once where we can do all of CManager create and hander register calls, and we call in it std::atexit where we call CManager_close. We can discuss tomorrow.

Well, I've rewritten and done this without std::atexit. Normal C++ shutdown calls the destructor for the singleton, so there's no need for atexit. BUT, two builds, ascent-nvhpc and el8-icc-ompi, are failing. I could ignore the former as an anomoly, but I think I have to dig into the latter. Both may be failing because the server is dying, but since that's run as a background ctest fixture, there's no indication if it dies.

Anyhow, you might look at the code as it stands and see if you like it better. I may not get to the failures today.

[anagainaru]

Not sure if this is why some builds fail but see below for a possible explanation.

[anagainaru]

I think a thread might reach this line before the thread calling the lambda finished in which case there will be a thread with an invalid internal ev state. If this is the thread that ends up calling std::call_once we will have a problem. Is there a reason why you want the CMregister_handler calls outside the singleton?

[eisenhauer]

The state should be complete once we have registered the formats. We don't need the handlers to have been registered until we get messages arriving. (I had them outside of the Singleton because they weren't visible inside it. I could have added the externs before it, but I don't think it should be necessary.). My understanding (which may be flawed), is that the singleton creation is thread safe, so anyone that gets it will get it after its constructor has been completed. So, the formats should have been registered and the internalEvState fully populated. So, the ev_state in every Remote instance should be appropriately populated. One of them will register the handlers, and from what I read of the call_once docs, there will be no race conditions. I.E. if a thread invokes the call_once, it will only proceed past that once someone has completed the call, even if it's some other thread doing it. I don't think there are holes here, but I can't explain the failures. Will be trying to get it into a docker image to see if I can get more info.

[eisenhauer]

Well, it wasn't the internal state thing. Instead icc seems not agree that this pattern produces a singleton. Specifically, the constructor gets called (creating the CManager), then the destructor gets called (closing the CManager), then we start registering handlers but get a segfault because the CManager has been closed and deallocated. I'm guessing that the other failure, from nvhpc is from the same problem. The gcc-based compilers seem happy, but not the rest.

[anagainaru]

Yes you are right, the internal state is guaranteed to be created when threads reach that line. Let me play with godbolt a bit to look at the assembly created by icc. I am very surprised only gcc likes this.

[anagainaru]

I still don't understand why the lambda doesn't behave like a mutex for icc/nvhpc but I'm giving up. Let's merge it like this and if I feel like spending some time on understanding this we can revisit in the future.