Deprecating SynchronizedMap/Buffer/...

[Mirco Dotta]

The following should be good enough reason for doing so

https://github.com/scala/scala/blob/master/src/library/scala/collection/mutable/SynchronizedMap.scala#L60

-- Mirco

---------------

Mirco Dotta

PSE-D, 1015 Lausanne, Switzerland

Twitter: @mircodotta

Build reactive apps

[Simon Ochsenreither]

What about all the Proxy stuff? The script.* stuff? The undo/revert/history stuff?

[Jason Zaugg]

On Wed, Aug 21, 2013 at 3:42 PM, Mirco Dotta <mirco...@typesafe.com> wrote:

The following should be good enough reason for doing so

https://github.com/scala/scala/blob/master/src/library/scala/collection/mutable/SynchronizedMap.scala#L60

I was a little shocked at how widely this is used:

https://github.com/search?l=scala&q=synchronizedmap&ref=searchresults&type=Code

Not just in toy projects, I spotted it in twitter/util.

Rather than deprecating, perhaps we ought to add all the methods (and of course a test case that keeps the complement of methods complete).

-jason

[Mirco Dotta]

On Aug 21, 2013, at 4:42 PM, Jason Zaugg <jza...@gmail.com> wrote:

On Wed, Aug 21, 2013 at 3:42 PM, Mirco Dotta <mirco...@typesafe.com> wrote:

The following should be good enough reason for doing so

https://github.com/scala/scala/blob/master/src/library/scala/collection/mutable/SynchronizedMap.scala#L60

I was a little shocked at how widely this is used:

https://github.com/search?l=scala&q=synchronizedmap&ref=searchresults&type=Code

Not just in toy projects, I spotted it in twitter/util.

TBH, I was shocked the other way around - how can it be part of the scala library?

Rather than deprecating, perhaps we ought to add all the methods (and of course a test case that keeps the complement of methods complete).

It doesn't seem manageable. It's too easy to forget to add/remove a method, and then we'd be back with the same problem.

Isn't this something that a @Synchronized macro annotation could solve? But even if that would help, macro annotation are not planned for 2.11 (are they?), 

and this is something I feel should be fixed in time for 2.11.

-jason

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

[Johannes Rudolph]

On Wed, Aug 21, 2013 at 4:42 PM, Jason Zaugg <jza...@gmail.com> wrote:

Rather than deprecating, perhaps we ought to add all the methods (and of course a test case that keeps the complement of methods complete).

But would that test notice if a new method is added on one of the superclasses exposing this problem again?  IMO the whole promise of the class that it would be possible to synchronize all its methods is wrong. Can you really say with confidence that it has no holes depending on how this trait is mixed into the final class? I'd also say it has to go.

--
Johannes

-----------------------------------------------
Johannes Rudolph
http://virtual-void.net

[Jason Zaugg]

On Wed, Aug 21, 2013 at 5:07 PM, Mirco Dotta <mirco...@typesafe.com> wrote:

On Aug 21, 2013, at 4:42 PM, Jason Zaugg <jza...@gmail.com> wrote:

It doesn't seem manageable. It's too easy to forget to add/remove a method, and then we'd be back with the same problem.

A test case could reflect on SyncMap and check that it at least overrides all methods that it inherits, other than a whitelist. So new a new method added in a base class would trigger a test failure.

 

Isn't this something that a @Synchronized macro annotation could solve? But even if that would help, macro annotation are not planned for 2.11 (are they?), 

and this is something I feel should be fixed in time for 2.11.

I agree, we shouldn't let the prospect of shiny new compiler features stop us from fixing things today with the tools at our disposal.

-jason

[√iktor Ҡlang]

My suggestion: Add missing methods and deprecate the SynchronizedX-classes.

Cheers,

√

--

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

--

Viktor Klang
Director of Engineering

Typesafe

Twitter: @viktorklang

[Johannes Rudolph]

On Wed, Aug 21, 2013 at 5:16 PM, Johannes Rudolph <johannes...@googlemail.com> wrote:

Can you really say with confidence that it has no holes depending on how this trait is mixed into the final class? I'd also say it has to go.

E.g. this: https://gist.github.com/jrudolph/6295996 

trait SecureMap[K, V] extends collection.mutable.Map[K, V] {

  override def size: Int = { 

    println("I'm safe!")

    super.size 

  } 

}

 


val a = new collection.mutable.ListMap[String, String] with SecureMap[String, String]

a.size

// I'm safe!

// res0: Int = 0

 


trait MyExtraSizeMap extends collection.mutable.Map[String, String] { 

  override def size = 42

}

 


val b = new collection.mutable.ListMap[String, String] with MyExtraSizeMap with SecureMap[String, String]

b.size

// I'm safe!

//res1: Int = 42

 


val c = new collection.mutable.ListMap[String, String] with SecureMap[String, String] with MyExtraSizeMap

c.size

//res2: Int = 42

 


trait SecureListMap[K, V] extends collection.mutable.ListMap[K, V] with SecureMap[K, V]

 


val d = new SecureListMap[String, String] with MyExtraSizeMap

d.size

//res3: Int = 42

 


val e = new SecureListMap[String, String] with MyExtraSizeMap with SecureMap[String, String]

e.size

//res4: Int = 42

[Alex Cruise]

I've been musing recently about rebuilding a nice RBAC (Role-Based Access Control) system I built a few years ago.  One thing that made it particularly nice for API developers was that the "what kind of API is this method" was declarative (using annotations) and the actual enforcement was done with AOP, so they couldn't forget to call the interceptor. Let's not have an argument about AOP, please. :)

Recently, I've been wondering if it might be possible to use a single macros definition to generate wrappers around multiple methods.  If this kind of duplication could be replaced with a macro, it would be great, and could solve this problem and a lot of related ones.

-0xe1a

--

[Erik Osheim]

On Wed, Aug 21, 2013 at 01:13:31PM -0700, Alex Cruise wrote:
> Recently, I've been wondering if it might be possible to use a single
> macros definition to generate wrappers around multiple methods. If this
> kind of duplication could be replaced with a macro, it would be great, and
> could solve this problem and a lot of related ones.

This is definitely possible. For example, Tom Switzer wrote a macro
for Spire which wraps arbitrary bits of code and adds overflow
checking (either throwing an Exception or returning Option):

import spire.macros.Checked.{checked, option}

def foo(x: Int, y: Int, z: Int) = checked { x * y + z }
foo(5, 6, 7) // 37
foo(500, 600, 700) // 300700
foo(50000, 60000, 70000) // ArithmeticOverflowException

def bar(x: Int, y: Int, z: Int) = option { x * y + z }
foo(5, 6, 7) // Some(37)
foo(500, 600, 700) // Some(300700)
foo(50000, 60000, 70000) // None

So, even beyond just wrapping an entire method, you could easily wrap
any block of code with a macro that would check permissions, access,
etc.

-- Erik

[Rex Kerr]

On Wed, Aug 21, 2013 at 11:07 AM, Mirco Dotta <mirco...@typesafe.com> wrote:

Rather than deprecating, perhaps we ought to add all the methods (and of course a test case that keeps the complement of methods complete).

It doesn't seem manageable. It's too easy to forget to add/remove a method, and then we'd be back with the same problem.

It's not manageable manually.  It's a little tooling to make it work automatically (at least to force you to pay attention), but that's what, a couple of hours of work?

The real problem is that

scala> val s = new collection.mutable.HashMap[Int,String] with collection.mutable.SynchronizedMap[Int,String]
s: scala.collection.mutable.HashMap[Int,String] with scala.collection.mutable.SynchronizedMap[Int,String] = Map()

scala> s.map{ case (k,v) => 2*k -> (v+v) }
res1: scala.collection.mutable.HashMap[Int,String] = Map(10 -> fishfish)

Oops.  Bye-bye, synchronization!

It's not a robust pattern.  In the absence of an upgrade that maintains mixins (not sure how, exactly--would require some builder generation, I guess), it needs to be its own class, not a mixin, or it is an invitation to fragility.

  --Rex

[Alex Cruise]

On Wed, Aug 21, 2013 at 1:38 PM, Erik Osheim <er...@plastic-idolatry.com> wrote:

This is definitely possible. For example, Tom Switzer wrote a macro

for Spire which wraps arbitrary bits of code and adds overflow
checking (either throwing an Exception or returning Option):

import spire.macros.Checked.{checked, option}

def foo(x: Int, y: Int, z: Int) = checked { x * y + z }

Cool! Does it suffer from the Bonfire of the Arities? :)

So, even beyond just wrapping an entire method, you could easily wrap
any block of code with a macro that would check permissions, access,
etc.

But what if you want to...  WRAP ALL THE METHODS? :)

-0xe1a

[Seth Tisue]

On Wed, Aug 21, 2013 at 5:00 PM, Rex Kerr <ich...@gmail.com> wrote:
> It's not a robust pattern. In the absence of an upgrade that maintains
> mixins (not sure how, exactly--would require some builder generation, I
> guess), it needs to be its own class, not a mixin, or it is an invitation to
> fragility.

couldn't agree more. it should be deprecated, not "fixed"

[Erik Osheim]

On Wed, Aug 21, 2013 at 05:02:39PM -0700, Alex Cruise wrote:
> Cool! Does it suffer from the Bonfire of the Arities? :)

I don't think so. It goes and detects any arithmetic operation on
types which could overflow (e.g. +) and adds checking. Doesn't have to
be an argument or return value:

import spire.macros.Checked.checked

def baz(x: String, y: String, z: String): (String, String) = checked {
((x.toInt * y.toInt).toString, (x.toInt * z.toInt).toString)
}

baz("2000", "3000", "4000") // (6000000,8000000)
baz("200000", "300000", "400000") // ArithmeticOverflowException

def qux(s: String): String = checked {
val x = s.toInt
val y = x * x
s + s
}

qux("10000") // 1000010000
qux("100000") // ArithmeticOverflowException

> But what if you want to... WRAP ALL THE METHODS? :)

Yeah, at that point we're talking compiler plugin. :/

-- Erik

[Adriaan Moors]

Yep. We'll deprecate all of the synchronized, proxy, scripted,... wrappers.

[Rex Kerr]

Wrappers are okay--you can write a little tooling to make sure (automatically) that they forward properly.

Mixins are not okay.

  --Rex

[martin odersky]

Synchronized maps seem to be necessary, no? So I vote for fixing them rather than deprecating them.

I am not (much) worried about

  scala> s.map{ case (k,v) => 2*k -> (v+v) }
  res1: scala.collection.mutable.HashMap[Int,String] = Map(10 -> fishfish)

as long as we make sure that Synchronized applies to mutable maps only. There are limits of composability to them, and that's OK. No need to pour out the baby with the bathwater.

Cheers

 - Martin

Martin Odersky
Prof., EPFL and Chairman, Typesafe
PSED, 1015 Lausanne, Switzerland
Tel. EPFL: +41 21 693 6863
Tel. Typesafe: +41 21 691 4967

[Paul Phillips]

On Thu, Aug 22, 2013 at 12:35 AM, martin odersky <martin....@epfl.ch> wrote:

Synchronized maps seem to be necessary, no? So I vote for fixing them rather than deprecating them.

Synchronized maps are necessary; synchronization via mixin override trait is unnecessary.

I am surprised anyone sees these traits as anything short of broken by design. There's no baby visible in there, it's all bathwater. If you want a synchronized map I can recommend one which doesn't have comments like "TODO: finish writing code" sprinkled around it, and the correctness of which doesn't depend on the ultra-fragile and untested interplay between an arbitrary number of classes.

  http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ConcurrentHashMap.html

[Adriaan Moors]

I agree with Paul. I don't think we've figured out how to use inheritance robustly for this kind of stuff.

[martin odersky]

On Thu, Aug 22, 2013 at 5:53 PM, Paul Phillips <pa...@improving.org> wrote:

On Thu, Aug 22, 2013 at 12:35 AM, martin odersky <martin....@epfl.ch> wrote:

Synchronized maps seem to be necessary, no? So I vote for fixing them rather than deprecating them.

Synchronized maps are necessary;

Yes, that's what I wanted to stress. 

 

synchronization via mixin override trait is unnecessary.

I agree that it would be great to have a better alternative here.  

Cheers

 - Martin

I am surprised anyone sees these traits as anything short of broken by design. There's no baby visible in there, it's all bathwater. If you want a synchronized map I can recommend one which doesn't have comments like "TODO: finish writing code" sprinkled around it, and the correctness of which doesn't depend on the ultra-fragile and untested interplay between an arbitrary number of classes.

  http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ConcurrentHashMap.html

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