epfl-lara / inox

Solver for higher-order functional programs, used by Stainless
Apache License 2.0
88 stars 20 forks source link

Move interrupted check lower in NonIncrementalSolver to avoid race conditions #161

Closed jad-hamza closed 3 years ago

jad-hamza commented 3 years ago

@samarion I'm moving the interrupt check a bit later to avoid the case where we could get interrupted right after the if check.

samarion commented 3 years ago

Maybe use some synchronization primitive in that case? You might run into JVM cache inconsistency issues otherwise.

jad-hamza commented 3 years ago

Which one do you think? I was having a look at AtomicReference, and store both the currentSolver and interrupt in a pair here, but I don't think it's ideal when you want to set just one component.

samarion commented 3 years ago

I think we can just use a synchronized block and make sure modifications of interrupted and currentSolver are atomic. Something like:

def interrupt(): Unit = {
  val optSolver = synchronized {
    interrupted = true
    currentSolver
  }
  for (solver <- optSolver) solver.interrupt()
}

override def check(config: CheckConfiguration): config.Response[Model, Assumptions] = {
  assert(currentSolver.isEmpty, ...)
  val newSolver = underlying()
  try {
    val runCheck = synchronized {
      currentSolver = Some(newSolver)
      !interrupted
    }
    if (runCheck) {
      ...
jad-hamza commented 3 years ago

I think we can just use a synchronized block and make sure modifications of interrupted and currentSolver are atomic.

Thanks, should be good now

jad-hamza commented 3 years ago

Thanks :)