Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stream.cons result not thread-safe #1220

Closed
scabug opened this issue Aug 13, 2008 · 10 comments
Closed

Stream.cons result not thread-safe #1220

scabug opened this issue Aug 13, 2008 · 10 comments
Assignees
Milestone

Comments

@scabug
Copy link

scabug commented Aug 13, 2008

The Stream object returned by cons evaluates its tail lazily, then caches the result in a member variable. Unfortunately, this evaluation can happen more than once if tail is called from multiple threads. If the tail function has state, then this can cause invalid results.

I think this behaviour should either be fixed - by synchronizing when evaluating/inspecting the tail - or documented somewhere. I've attached a script that demonstrates the problem and a rough solution.

@scabug
Copy link
Author

scabug commented Aug 13, 2008

Imported From: https://issues.scala-lang.org/browse/SI-1220?orig=1
Reporter: @richdougherty
Attachments:

@scabug
Copy link
Author

scabug commented Aug 13, 2008

@DRMacIver said:
The correct solution to this is probably just to update Stream to use the new(ish) laziness features, which are threadsafe.

@scabug
Copy link
Author

scabug commented Aug 14, 2008

@richdougherty said:
Replying to [comment:1 DRMacIver]:

The correct solution to this is probably just to update Stream to use the new(ish) laziness features, which are threadsafe.

That's a good idea in theory. The trouble is that the Stream needs to be aware of whether or not the tail has been evaluated (see hasDefiniteSize, addDefinedElems). I don't think it's possible to find that out when using the 'lazy' keyword. Or am I wrong in making this assumption?

@scabug
Copy link
Author

scabug commented Aug 14, 2008

@DRMacIver said:
It's not, though I've often wished for that capability. What I tend to do in that context is add an additional boolean parameter "thunked" or something like that, with:

var thunked = false;
lazy val foo = {thunked = true; fooCalc }

It would be nice if you could access the functionality to determine if the value had been thunked without needing to recreate it yourself though.

@scabug
Copy link
Author

scabug commented May 5, 2009

@paulp said:
Replying to [comment:3 DRMacIver]:

It would be nice if you could access the functionality to determine if the value had been thunked without needing to recreate it yourself though.

You're wasting a perfectly good bit there, bub!

scala> class Foo { lazy val bar = 5 }                                           defined class Foo

scala> val x = new Foo                        
x: Foo = Foo@c7f3e3

scala> val f = x.getClass.getField("bitmap$$0")
f: java.lang.reflect.Field = public volatile int Foo.bitmap$$0

scala> def isEvaluated = (f.getInt(x) & 1) != 0
isEvaluated: Boolean

scala> isEvaluated
res0: Boolean = false

scala> x.bar
res2: Int = 5

scala> isEvaluated
res3: Boolean = true

Which is not to say this approach doesn't have a certain fragility and oh so slight implementation dependence... but I see no reason there couldn't be an interface to the data.

@scabug
Copy link
Author

scabug commented Jun 1, 2010

@paulp said:
See also #3515 for a little more recent discussion.

@scabug
Copy link
Author

scabug commented Jun 3, 2010

@oschulz said:
[PATCH] Replaced Stream.Cons with a thread-safe version.

@scabug
Copy link
Author

scabug commented Jun 3, 2010

@oschulz said:
Since there's work going on with Stream at the moment anyway, might this be a good time to fix this on trunk (even if it doesn't make it into 2.8.0)?

I've appended a patch with a proposed solution (passes ant test on top of current r22157).

It replaces Stream.Cons by

  @serializable @SerialVersionUID(2734764611255889683L)
  final class Cons[+A](hd: A, tl: => Stream[A]) extends Stream[A] {
    override def isEmpty = false
    override val head = hd
    def tailDefined = tailDefinedVar
    @volatile private var tailDefinedVar = false
    override lazy val tail: Stream[A] = {
      tailDefinedVar = true
      tl
    }
  }

I regenerated the serial version UID, since this will be incompatible with the old one, right?

@scabug
Copy link
Author

scabug commented Jul 22, 2010

@oschulz said:
Now that 2.8.0 is released, can we fix this?

@scabug
Copy link
Author

scabug commented Sep 1, 2010

@dragos said:
(In r22897) Closes #1220. Stream.tail is now thread safe. Review by odersky.

@scabug scabug closed this as completed May 18, 2011
@scabug scabug added this to the 2.8.1 milestone Apr 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants