Update Julia CI job, and add MultiObjectiveAlgorithms as a test

[odow]

An attempt to reproduce the segfaults in jump-dev/MultiObjectiveAlgorithms.jl#211, where we can additionally make changes to HiGHS.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.16%. Comparing base (8e8d4f6) to head (4032c5f).
⚠️ Report is 1 commits behind head on latest.

Additional details and impacted files

@@            Coverage Diff             @@
##           latest    #3136      +/-   ##
==========================================
- Coverage   73.16%   73.16%   -0.01%     
==========================================
  Files         430      430              
  Lines      103967   103964       -3     
  Branches    16718    16719       +1     
==========================================
- Hits        76069    76066       -3     
  Misses      27622    27622              
  Partials      276      276              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[odow]

@filikat this branch now uses HiGHS#latest to run the Julia tests, as well as the tests from jump-dev/MultiObjectiveAlgorithms.jl#211 (not that I can reproduce the segfault there though).

I wonder if it's something like this:

1. We start one HiGHS instance
2. We run, do a bunch of stuff
3. We start a new HiGHS instance
4. We run, do a bunch of stuff
5. Sometime during (4), the GC runs and collects the first HiGHS instance, including calling Highs_destroy.

[filikat]

This is totally possible. In the C api, Highs_destroy not only deletes the Highs instance, but also kills the parallel scheduler. The scheduler relies on static variables, so there is only one of it, regardless of how many Highs instances are created. If you create two Highs instances, destroy the first and still use the second, this may cause issues because the parallel scheduler is terminated while the second instance is still using it.

I don't think that this should happen. The destructor of the static instance of ExecutorHandle already kills the scheduler at process termination, so technically there is no need to shut it down explicitly. I wonder if calling shutdown explicitly is done to solve some of the issues that we were having on Windows. Maybe @galabovaa @jajhall have some insights into this.
Maybe we should have a Highs_finalise function in the C api that does not depend on any instance and simply kills the scheduler, so that it is not done by Highs_destroy.

Tomorrow I will check if any of the unit tests creates this exact situation of two separate instances where one is destroyed before the other. If not, I will write such a test and try to verify if the issue is there.
One easy way to check is also to comment the line Highs::resetGlobalScheduler(true); in Highs_destroy.

[odow]

One easy way to check is also to comment the line Highs::resetGlobalScheduler(true); in Highs_destroy.

Ooooooo.

HiGHS/highs/interfaces/highs_c_api.cpp

Lines 176 to 178 in 04024d7

	void Highs_destroy(void* highs) {
	Highs::resetGlobalScheduler(true);
	delete (Highs*)highs;

Based on this scary warning, it seems highly likely that's the cause:

HiGHS/highs/interfaces/highs_c_api.h

Lines 2494 to 2495 in 04024d7

	* It is not thread-safe to call this function while calling `Highs_run` or one
	* of the `Highs_XXXcall` methods on any other Highs instance in any thread.

I may be able to fix this in JuMP, because we explicitly mark that the GC can run during a call to Highs_run:

https://github.com/jump-dev/HiGHS.jl/blob/5f3e3b7a6af3b988186f64934416f0241d5eefb2/src/MOI_wrapper.jl#L2295-L2297

[odow]

Added by #2493

There is already oid Highs_resetGlobalScheduler(const HighsInt blocking); in the C API.

The C API should never call resetGlobalScheduler because you can never do so in a thread-safe way. This must be called only by the user.

[odow]

So I think this was probably the cause of the failures in #2998. I should have taken a deeper look, instead of just working around the cause.

[odow]

I'm fairly happy that resetGlobalScheduler was the culprit. Once #3141 is merged I'll update this PR to be just the changes for the Julia CI tests.

[filikat]

The C API should never call resetGlobalScheduler because you can never do so in a thread-safe way. This must be called only by the user.

I agree. Indeed, the cpp functions that run Highs never call resetGlobalScheduler explicitly. It is always called by the user after Highs has finished running.

[jajhall]

Am I right in thinking that nothing is lost by removing 'resetGlobalScheduler' from the C API method that destroys HiGHS.

All we need is a C API method that calls 'resetGlobalScheduler' so that we, and users, can achieve this from C. Something we'll need for CI tests?

[filikat]

There already is such a function, Highs_resetGlobalScheduler.
We need to make sure that users are aware that this should be called by them when they are finished using all Highs instances.

[filikat]

Am I right in thinking that nothing is lost by removing 'resetGlobalScheduler' from the C API method that destroys HiGHS.

I think so. It was added to avoid some valgrind errors when running the minimal C example, but the same can be achieved by calling Highs_resetGlobalScheduler explicitly in call_highs_from_c_minimal.c, like Oscar did.

Something we'll need for CI tests?

The tests in #3141 are passing, so I don't think there's much else to fix.