scipopt/russcip

A more robust solution might be to add a lifetime directly to `ScipPtr`. So have `OwnedScip` and `ScipRef<'a>` instead of the `consumed` field.

mmghannam opened this issue · 8 comments

mmghannam commented 
          A more robust solution might be to add a lifetime directly to `ScipPtr`. So have `OwnedScip` and `ScipRef<'a>` instead of the `consumed` field.

Originally posted by @Andful in #149 (comment)

Andful commented

Oh, was not expecting the pull request to be merged without sparking any discussion.

I am a bit busy currently to implement this , but maybe, just to get an idea of the scope, I have a few questions:

  • What is the purpose of clone_for_plugin? From my understanding it is used in callbacks to query model information, e.g. during branching.
  • Is it needed? I think, as of now, it suffers from the same problems as in #138 (By accessing the clone after the main model is dropped). I think it might be better just to remove it all together. If someone wants a shallow-copy(or a reference), the user can either choose to use a reference (&Model) or a reference-counting pointer (Rc<Model> & Arc<Model>).
  • Would it be fine to remove it (given that I fix the tests)? It would be an API breaking change :/, and some users might not love it.
mmghannam commented
  • What is the purpose of clone_for_plugin? From my understanding it is used in callbacks to query model information, e.g. during branching.

Yes your understanding is correct.

  • Is it needed? I think, as of now, it suffers from the same problems as in segfault (use after free) when accessing solution after model is dropped #138 (By accessing the clone after the main model is dropped). I think it might be better just to remove it all together. If someone wants a shallow-copy(or a reference), the user can either choose to use a reference (&Model) or a reference-counting pointer (Rc<Model> & Arc<Model>).
  • Would it be fine to remove it (given that I fix the tests)? It would be an API breaking change :/, and some users might not love it.

I don't mind removing it at all, as long as access to the SICP model is still allowed from plugins (callbacks).

Sorry for the late reply and thank you so much for your efforts!

mmghannam commented

@Andful what do you think about wrapping up ScipPtr in an Arc and passing it around every thing that has a reference to the model, and changing all ScipPtr methods to use only immutable reference, since they are anyway can be protected from the Models methods. Then that should eliminate the use after free errors and would not need lifetimes? do you foresee any issues with this?

Andful commented

That seems like a much easier solution. Was contemplating that. I have to think about it a bit. We can defer freeing to be done by ScipPtr, What is the function of vars and conss in e.g. Solution?

mmghannam commented

I have started implementing this on the branch arc-scipptr if you want to take a look.

What is the function of vars and conss in e.g. Solution?

I assume you mean in the Model states not the solution, it's to avoid creating structs for variables and constraints every time the user queries them. But now I'm actually rethinking this I think the rust compiler could optimize that away, and would remove this internal state that needs to be kept up to date. Anyhow, that's a separate issue.

Andful commented

I cannot see the branch. But feel free to ping me if you make a pull request. I was asking about vars and conss because they are freed by Model. If we let scipptr free the model and the variables on the C side, we just have to be a bit careful that vars and conss are not used after free or double freed.

Regarding Arc<ScipPtr>, I have a small comment. ScipPtr is not Send. If there is no plan on making ScipPtr Send I would do Rc<ScipPtr>, but if it is planned then Arc is fine.

mmghannam commented

I cannot see the branch. But feel free to ping me if you make a pull request. I was asking about vars and conss because they are freed by Model. If we let scipptr free the model and the variables on the C side, we just have to be a bit careful that vars and conss are not used after free or double freed.

Sorry I forgot to push my changes :) I opened a draft PR with it #155.
Since the free method will only be called once after the last thing that has a reference in the model is freed, then use-after-free shouldn't happen. Also, because only the drop method in ScipPtr is the one responsible for freeing variables and constraints then we should be ok if we use Arc since it will only be called once.

Regarding Arc<ScipPtr>, I have a small comment. ScipPtr is not Send. If there is no plan on making ScipPtr Send I would do Rc<ScipPtr>, but if it is planned then Arc is fine.

I think I want it to be Send in the future, I just need to think what invariants need to hold for this to be safe.

mmghannam commented

Update: First step in this was done in #155