Race condition in SessionVar

[Piotr Dyraga]

Hello,

I have recently worked on some piece of multithreaded code that access SessionVar concurrently. No rocket science here - one thread sets SessionVar’s value and other threads possibly reading it at the same time. We can also treat these threads just as actors.

What hit me was that *sometimes* SessionVar’s value "magically" comes back to the default one although it was already set to some other. We’ve looked at this issue with Antonio and we think that there is a race condition between is and apply methods. doSync wraps the work in is method but there is no such wrap in apply. As a result, when is and apply executions interleaves, SessionVar’s value can be brought back to the default one. Here is a spec that occasionally fails because of this issue. It uses 11 threads but we were able to reproduce this issue with just two threads in our code.

import net.liftweb.actor.LAFuture
import net.liftweb.common.Empty
import net.liftweb.http.{SessionVar, S, LiftSession}

class SessionVarRaceSpec extends WebSpec {

  object TestSessionVar extends SessionVar[String]("Uninitialized")

  val session = new LiftSession("/", "", Empty)

  "SessionVar" should {

    "let to safely update its state from multiple threads" withSFor("/", session) in {
      for (_ <- 1 to 10) {
        LAFuture.build {
          S.initIfUninitted(session) {
            println("TestSessionVar is " + TestSessionVar.is)
          }
        }
      }

      LAFuture.build {
        S.initIfUninitted(session) {
          println("SET TestSessionVar to " + TestSessionVar("SomeValue"))
          println("TestSessionVar is " + TestSessionVar.is)
        }
      }

      TestSessionVar.is must eventually(beEqualTo("SomeValue"))
    }
  }
}

Example of failed execution:

> test:testOnly *.SessionVarRaceSpec
SET TestSessionVar to SomeValue
TestSessionVar is Uninitialized
TestSessionVar is Uninitialized
TestSessionVar is Uninitialized
TestSessionVar is Uninitialized
TestSessionVar is Uninitialized
TestSessionVar is Uninitialized
TestSessionVar is Uninitialized
TestSessionVar is Uninitialized
TestSessionVar is Uninitialized
TestSessionVar is Uninitialized
TestSessionVar is Uninitialized
[info] SessionVarRaceSpec
[info] 
[info] SessionVar should
[error]   x let to safely update its state from multiple threads
[error]    'Uninitialized' is not equal to 'SomeValue' (SessionVarRaceSpec.scala:31)
[info] 
[info] 
[info] 
[info] Total for specification SessionVarRaceSpec
[info] Finished in 4 seconds, 325 ms
[info] 1 example, 1 failure, 0 error
[info] 
[error] Failed: Total 1, Failed 1, Errors 0, Passed 0
[error] Failed tests:
[error]  com.ontheserverside.lib.SessionVarRaceSpec
[error] (test:testOnly) sbt.TestsFailedException: Tests unsuccessful
[error] Total time: 7 s, completed Sep 16, 2016 11:01:53 AM

Thanks,

Piotr

[Antonio Salazar Cardozo]

The most frustrating thing here is that to properly synchronize this we'd likely need

to wrap all of `apply` in `doSync`, which seems painful.

Notably, `setIfUnset` is wrapped in `doSync`. So perhaps the intent is for `setIfUnset`

to be used in these cases, while `apply` should be used in cases where you generally

know that the SessionVar will always be read after it is set.

Thoughts?

Thanks,

Antonio

[Matt Farmer]

It seems like (if that’s the case) the apply variant should indicate it’s riskyness with a ! at the end, right?

—
Matt Farmer | Blog | Twitter
GPG: CD57 2E26 F60C 0A61 E6D8  FC72 4493 8917 D667 4D07

--
--
Lift, the simply functional web framework: http://liftweb.net
Code: http://github.com/lift
Discussion: http://groups.google.com/group/liftweb
Stuck? Help us help you: https://www.assembla.com/wiki/show/liftweb/Posting_example_code

---
You received this message because you are subscribed to the Google Groups "Lift" group.
To unsubscribe from this group and stop receiving emails from it, send an email to liftweb+u...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Antonio Salazar Cardozo]

Well, `apply` is a shortcut for `invoke with ()`, so if we did that it would no longer be callable

using `()`. Could make `setIfUnset` the `apply` default and `set_!` or something of the sort

be the other version.

I do think it's likely fairly rare for session vars to be read in a way that they could conflict

with a set operation as well, though... Needs more thought...

Thanks,

Antonio

To unsubscribe from this group and stop receiving emails from it, send an email to liftweb+unsubscribe@googlegroups.com.

[Piotr Dyraga]

Worth noting: it's not only about `SessionVar`, `RequestVar` is affected as well, just like any other implementation of `AnyVarTrait`. (It hit me in tests for session/request aware futures)

My personal opinion is that when I have request or session variable in hand, I'd expect all read and write operations to be thread safe. This is subjective opinion, though. Maybe I'm just too lazy ;-)

To unsubscribe from this group and stop receiving emails from it, send an email to liftweb+u...@googlegroups.com.

[Antonio Salazar Cardozo]

In general I do think you're right. But I'm also loath to impose a potentially large

performance hit to applications that understood the current semantics, with no

way of improving them. We've also run out of runway to make this change for

Lift 3, so I don't love the idea of changing something fundamental like this—even

though I think the current behavior can also be confusing.

Would love to hear some other thoughts on the matter.

Thanks,

Antonio