Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

An attempt to fix potential deadlock issues in Windows. #1923

Closed

Conversation

mathgeekcoder
Copy link
Contributor

Removes synchronization with worker threads on shutdown. Also removes the "search" for the main executor in the worker threads.

Instead we simply pass the main executor to the thread as a parameter. We also pass the underlying shared_ptr to avoid potential edge cases where reference count drops to zero before some threads initialize.

I made the run_worker static to avoid any confusion about this vs executor->ptr, and so it uses the shared_ptr to reference the shared memory.

The last worker thread will delete the shared memory, via the shared_ptr reference count.

This might fix issues mentioned in #1886 and #1044.

Removes synchronization with worker threads on shutdown. Also removes the "search" for the main executor in the worker threads.

Instead we simply pass the main executor to the thread as a parameter.  We also pass the underlying shared_ptr to avoid potential edge cases where reference count drops to zero before some threads initialize.

I made the run_worker static to avoid any confusion about "this" vs "executor->ptr", and so it uses the shared_ptr to reference the shared memory.

The last worker thread will delete the shared memory, via the shared_ptr reference count.
@mathgeekcoder
Copy link
Contributor Author

Sample C++ code that reproduces the error on windows.

std::array<double, 2> lb = { -kHighsInf, -kHighsInf };
std::array<double, 2> ub = {  kHighsInf,  kHighsInf };

std::array<HighsInt, 2> index = { 0, 1 };
std::array<double, 2> value = { 0, 1 };

std::array<double, 2> lower = { 2, 0 };
std::array<HighsInt, 2> starts = { 0, 2 };
std::array<HighsInt, 4> indices = { 0, 1, 0, 1 };
std::array<double, 4> values = { -1, 1, 1, 1 };

Highs h;
h.setOptionValue("output_flag", false);
h.setOptionValue("threads", 20); // setting to large number makes it more likely to deadlock

h.addVars(2, lb.data(), ub.data());
h.changeColsCost(2, index.data(), value.data());
h.addRows(2, lower.data(), ub.data(), 4, starts.data(), indices.data(), values.data());

std::cout << "starting" << std::endl;

for (int i = 0; i < 100; i++) {
   std::thread([&] { h.run(); }).join();
   std::cout << i << std::endl;
}

std::cout << "Done" << std::endl;

@mathgeekcoder mathgeekcoder marked this pull request as draft September 13, 2024 00:53
…ensure shared memory is kept alive until all threads have finished.

Changed identification of main thread to use std::thread::id rather than thread_local memory pointer.

Added vector of workerThreads which can be detached or joined on shutdown.

Ensured that shutdown can only block if called on main thread, otherwise it might be possible to deadlock.

Manually using cache_aligned memory allocation, it was used previously with shared_ptr and I wanted to keep it just in case.

Note: I had some weird issues when compiling with /MD flag with mvsc.  It would run but often crash.  /MT flag works consistently for me.
@mathgeekcoder mathgeekcoder marked this pull request as ready for review September 14, 2024 00:04
@galabovaa
Copy link
Contributor

Closing, changes are now in #1948

@galabovaa galabovaa closed this Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants