[WIP] Fix the infinite cycle in Lazy Vals

[DarkDimius]

Reported by Andrzej Plutecki(@baszo) in https://groups.google.com/forum/#!topic/dotty-internals/3LMNItLQw-A
I haven't seen this happen in practice in hours and hours of benchmarking, but this is indeed a formal bug.

[DarkDimius]

@baszo please review.

[baszo]

This will not fix @the problem. Because when other thread will set flag to 3 function STATE(cur, ord) that you used with variable cur will be still set to old state in such solution it is needed to reassure and use get function to get new state

[DarkDimius]

@baszo, nice catch! it has been long time since I have written this code, so I forgot that STATE doesn't read the state. Could you have a look at the updated fix?

[baszo]

Still there is a problem. Last thing to make it working is a little change with setFlag function.
if (STATE(cur, ord) == 1) retry = CAS(t, offset, cur, v, ord)
just remove 'retry ='
Because while 2 thread will use CAS simultaneously one will set 1 to 2 and other 1 to 3. First win and with new update if statement will pass and call wait. In thread that initialized value CAS failed and retry will be set to false and then notifyAll will never be called

[DarkDimius]

@baszo, I'm not sure if I understood you.
In the current implementation:

• before sleeping thread should succeed to set flat from 1 to 2, synchronize and check that it is still 2;
• in order to not call notify, thread should succeed to set flag from 1 to 3.

If I understood the case you are describing, thread A that succeeds change flag 1 -> 2, and starts syncrhonization. By the time it syncrhonized we know that thread B cannot successfully change 1 -> 3, as the change 1 -> 2 has been passed. This means that the thread B will in any case perform

• notifyAll inside syncrhonized block;
• change 2 -> 3;

We have to cases to consider here, either thread A is faster to grab the lock, or thread B is faster to grab the lock.

If thread A is faster to grab the lock, it will be woken by notify call when thread B invokes it.
If thread B is faster to grab the lock, by the time thread A has grab the lock it will not observe the 2 state anymore, because thread B set it to 3 before going into synchronized block.

Is there something I'm missing?

[DarkDimius]

BTW: While I still don't see the issue you are describing, solution that you are proposing will lead to infitine recursion in absence of contention. The (state == 1) state has to modify retry, because (state == 2) may never be reached.

[DarkDimius]

While we at it, I think it's worth to refactor the LazyVals module and document the assumptions in it. Marked WIP for this reason.
I should be able to do it next week, after I'm back from ScalaDays. If some-one volunteers to help with it I can review before.

[baszo]

There is 3 case.
Thread A is initializing value and set flag to 1. Thread B got state 1 so it will go to wai4nottification function. Then thread A finished initializing and still function got will return state 1 and will go to setFlag. So both A and B thinks that flag state is 1. Both of them want by using CAS to set flag (A) 1->2 and (B) 1->3. One of them will fail. If it will be B it is ok. But if A then CAS will return false and break while loop so notifyall will not be executed and B will wait.

[DarkDimius]

@baszo, are you able to reproduce the issue? what hardware are you using?
I'm not suggesting that there's no bug, but it would be nice if we can add the test, both to help me to understand the case you are describing and make sure that the problem stays fixed.

[baszo]

I used to reproduce this problem this code:

import scala.concurrent.ExecutionContext.Implicits.global
import scala.concurrent._
import scala.concurrent.duration._

object A {
  lazy val base: Int = 42
  lazy val start = B.step
}

object B {
  lazy val step: Int = A.base
}

object ProblemScenario {
  def run = {
    val result = Future.sequence(Seq(
      Future { A.start },                        // (1)
      Future { B.step }                          // (2)
    ))
    Await.result(result, 1.minute)
  }
}

[baszo]

Ok i understand what you meant. Solution i mentioned will work and solve the problem but always will call notifyAll so it is inefficient. But do you understand the problem or should i try to describe it better? I can create some call stack of methods to show this particular case.

[baszo]

I though that through and I think the problem is as I mentioned in SetFlag function. When state == 1 it will try to use CAS and i think it should be
if (STATE(cur, ord) == 1) retry = !CAS(t, offset, cur, v, ord)

because when CAS will fail (that means other thread changed state) it should not quit loop but try again, to get changed state and redo operation of setting new state
Combined with your fix about reassurance about state before waiting it will do the job

[felixmulder]

@DarkDimius - this branch is on the dotty repo and not your own fork / staging. For the releases, I'd like to remove it as soon as possible.

If you have too much on your plate, I'll push it to staging and re-open the from the correct downstream.

[DarkDimius]

@felixmulder

If you have too much on your plate, I'll push it to staging and re-open the from the correct downstream.

Sounds good!

[Blaisorblade]

OK, I convinced myself the bugs discussed here are fixed, which makes me rest easier.

Looking again at @baszo's scenario there's a typo I think, but otherwise this does describe the bug in SetFlag, so the fix looks good (not sure about other bugs). The fixed version would be:

Thread A is initializing value and set flag to 1. Thread B got state 1 so it will go to wai4nottification function. Then thread A finished initializing and still function got will return state 1 and will go to setFlag. So both A and B thinks that flag state is 1. Both of them want by using CAS to set flag (A) 1->3 and (B) 1->2. One of them will fail. If it will be B it is ok. But if A then CAS [in setFlag] will return false and break while loop so notifyall will not be executed and B will wait.