chkwon / PATHSolver.jl

provides a Julia wrapper for the PATH Solver for solving mixed complementarity problems
http://pages.cs.wisc.edu/%7Eferris/path.html
MIT License
50 stars 15 forks source link

Preserve data handed to callbacks with pointer_from_objref #113

Closed lassepe closed 4 months ago

lassepe commented 4 months ago

Currently, pointer_from_objref is used in a way that does not strictly guarantee that that object will still be alive by the time it is used on in C. This PR introduces a _preserved_pointer_from_objref variant that pushes the object to a global IdSet to make sure the data remains referenced in Julia.

Note: I am currently clearing the dict at the bottom of the solve function. However, the chosen design with a global dict may leak memory if the constructors that call into _preserved_ponter_from_objref are called outside of the context of solve_mcp. So an alternative design may be preferred where the _preserved_context is handed explicitly to the functions doing the pointer conversion. This PR follows the discussion on slack:

https://julialang.slack.com/archives/C688QKS7Q/p1714149796226809

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 97.36842% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 98.07%. Comparing base (d6fc14e) to head (26c1ad1).

Files Patch % Lines
src/C_API.jl 97.36% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #113 +/- ## ======================================= Coverage 98.06% 98.07% ======================================= Files 3 3 Lines 414 416 +2 ======================================= + Hits 406 408 +2 Misses 8 8 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

odow commented 4 months ago

I need to take a look at this. I think I also need to add nightly test so that we can see it failing with the current setup.

odow commented 4 months ago

Closing in favor of https://github.com/chkwon/PATHSolver.jl/pull/114. The global dict isn't appropriate because there will be a conflict if multiple solve_mcp are called from different threads (even if, at the moment, PATH is not thread safe, there is no reason to make it even less-so).