[BUG] Segmentation fault in interruptible.hpp

[cjnolet]

cugraph has been encountering a segfault in interruptible.hpp. It seems to only appear when taking the Python path through their code, for some reason, but it's very consistent. The reproducer for this issue (assumes #1224 has been reverted) is:

import cugraph
import cudf
if __name__ == "__main__":

    edgelist = cudf.DataFrame( { "src": [0, 1, 2], "dst": [1, 2, 4], "wgt": [0.0, 0.1, 0.2], })
    G = cugraph.Graph()

    G.from_cudf_edgelist(edgelist, source="src", destination="dst", edge_attr="wgt", store_transposed=True)

    r = cugraph.pagerank(G)

And it's important to run this in a loop because it doesn't happen everytime:

while true; do python test_failure.py; if [$? -ne 0]; then break; fi; done

The following shows the error happening on line 214 here, which appears it might be related to an object being used in a callback after it's been cleaned up (something like registry_.find(xxx) after registry_ has already been deallocated, maybe?)

cc @achirkin @tfeher

I'm not sure why this doesn't seem to be happening in pylibraft, cuml, or cuopt but it's definitely consistently reproducible in cugraph.

@ChuckHastings @alexbarghi-nv @rlratzel @seunghwak @BradReesWork FYI

[achirkin]

Does cugraph run something in multiple processes there? The registry_ is a static, global per-process variable, and the error seems to be happening at __run_exit_handlers, which happens at the program exit.
A wild guess: could it be related to the order of destruction between the registry_ and the mutex_? What if the swap the two lines in here?

raft/cpp/include/raft/core/interruptible.hpp

Lines 181 to 184 in 6a7e125

	/** Global registry of thread-local cancellation stores. */
	static inline std::unordered_map<std::thread::id, std::weak_ptr<interruptible>> registry_;
	/** Protect the access to the registry. */
	static inline std::mutex mutex_;

(update: no, this didn't work)

[cjnolet]

@achirkin, I'm wondering if we could use somehting at the global level or non-member thread-local level like a std::atomic<bool> flag that could be flipped to true when the deleter is called on the registry_? Can you think of any other way we might be able to check for this state so we can handle it gracefully?

cc @jrhemstad for thoughts as well- it looks like we have a race condition happening in the callback for interruptible where it's trying to use registry_ after it's already been deleted. The order of deconstruction for static members seems to be inconsistent from behavior that @seunghwak has been seeing.

[seunghwak]

As I mentioned in the slack thread,

This is due to the arbitrary destruction order between s and registry_.
https://github.com/rapidsai/raft/blob/branch-23.02/cpp/include/raft/core/interruptible.hpp#L134
https://github.com/rapidsai/raft/blob/branch-23.02/cpp/include/raft/core/interruptible.hpp#L182

It seems like there is no guaranteed destruction order between a thread local static variable defined inside a member function and static class member variables.

If s is destroyed after registry_ is destroyed, registry_ will be accessed after destruction.

I confirmed this is indeed happening by defining

class MyClass {
 public:
  MyClass() { std::cout << "MyClass() called" << std::endl; }
  ~MyClass() { std::cout << "~MyClass() called." << std::endl; }
};

and adding static inline MyClass my_class_; after registry_ and mutex_ (https://github.com/rapidsai/raft/blob/branch-23.02/cpp/include/raft/core/interruptible.hpp#L184).

Also added the printout statement in the custom deleter (https://github.com/rapidsai/raft/blob/branch-23.02/cpp/include/raft/core/interruptible.hpp#L213); this custom deleter will be called when s is destroyed.

In testing, the print statement from ~MyClass sometimes appears before the print statement from the custom destructor, meaning the custom destructor can access registry_ after destruction.

[achirkin]

Here's a rather ugly, but seemingly working, fix #1229 - wrap the registry_ in a shared pointer and use weak_ptr.lock() to avoid accessing it after it's been deleted.