This will break the Windows build and is not exception safe. We should use std::mutex from C++11 instead of pthread mutexes. That will probably require changing at least THTensorRandom.c to C++.

To clarify on the exception issue: a lot of functions in TH can call THError, including the TH_TENSOR_APPLY macros. In PyTorch, THError throws a C++ exception. If that happens while the pthread lock is held, it would not be released. That would lead to deadlocks if the random function were called again.

I suspect that moving all of TH to C++ would be a pain. You can avoid the C++ types in the THRandom.h header by using an opaque struct in the header file.

@yf225 can you finish this this week, i think folks are blocked on it.

@colesbury What do we think of using different generators for multi-threads (as @Stonesjtu suggested in #3794 (comment))? Perf-wise it might be faster than using mutex because we are not waiting for the lock at all, not sure about implementation yet.

@yf225 the PR corresponding to that was closed. We should just finish this as it currently stands and merge.

I think thread local generators are not a good idea because it’s easy to forget to seed a worker thread

@colesbury It seems that if I do it as an opaque struct pointer and use new to initialize the struct in the THRandom.cpp file, then I am not sure if I should call delete on this pointer in THGenerator_free, because this THGenerator can be copied elsewhere and deleting the pointer here will destroy the mutex reference in those copies.

I guess moving TH to C++ land might be a cleaner way to go, although the code change would be quite big.

Another issue is that if you want to actually acquire the mutex from C you are going to have to work hard to get unwinding to actually free it when necessary.

This still has the issue that it's not exception safe. An exception thrown after the lock call will not release the lock, causing deadlocks the next time a random function is called.

We should covert the C files to C++ and use std::lock_guard.

We don't need to convert all of TH in one go. I think converting THTensorRandom.c to .cpp should be sufficient.

@colesbury I think if we put the std::mutex in THRandom.h, then any file that includes this header would need to be converted to C++ as well?

@colesbury If we change THTensorRandom.c to .cpp, we probably also need to change THTensor.c (that contains #include "generic/THTensorRandom.c") to .cpp, and that will require us to change many of other dependent TH files to C++ as well.

Haven't figured out what's the appropriate solution to this yet, or if adding try-catch block to catch THError would be a feasible approach.

@yf225, yes. I think your current trick in THRandom.cpp can work. Move the definition of mutex_s to a separate header file that's included by THRandom.cpp and THTensorRandom.cpp. THRandom.h can keep the opaque definition.

As Adam Paszke points out, you can also simplify the definition of mutex_s a bit.

I don't think a try-catch block is a good solution. We should be moving towards using C++, even if it requires more work.

After changing things to C++, https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/test/scalar_tensor_test.cpp#L214 seems to fail when lhs is empty and rhs is a tensor with single element 0.

The error message is:

terminate called after throwing an instance of 'std::runtime_error'
 what():  invalid argument 2: out of range: 0 out of 0 at /private/home/willfeng/pytorch/aten/src/TH/generic/THTensorMath.cpp:382
../aten/tools/run_tests.sh: line 14: 23523 Aborted                 $BUILD_ROOT/src/ATen/test/scalar_tensor_test

In the normal case, TRY_CATCH_ELSE is expected to catch this std::runtime_error thrown from THTensorMath.cpp, but it wasn't the case here. I manually tested it and it seems that the catch block was not entered at all.

Any suggestions on how I should go about debugging this?

EDIT: fixed in #4857.

There are a lot of unused variable warnings in TH after moving things to C++. I can fix them here or submit another PR for it.

I think this breaks C FFI plugins since those are built with C, rather than C++. Specifically, I get this error:

packages/torch/utils/ffi/../../lib/include/TH/THRandom.h:6:17: fatal error: mutex: No such file or directory
     #include <mutex>
                     ^
    compilation terminated.
    error: command 'gcc' failed with exit status 1

CC @yf225 @colesbury @soumith ^^

Drat! I guess, if we want to support FFI, we're going to need some C extern blocks and a C-compatible header for FFI plugins to import T_T

Yeah, I actually wanted to write my header file in C++, but parser didn't support extern "C" and/or namespace constructs.

There’s a new C++-based extensions system underway. It might be simpler to recommend moving to that, but we should figure out an easy way to migrate (since this is breaking)

@apaszke, I'd love to try new C++ extension out when it's ready. That said, would you support the C way for a while for backward compatibility?

yes we will support the C extension. We'll issue fixes shortly on this.

@alsrgv now fixed in master

@soumith, thanks for the quick turnaround!