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

Jump to bottom

Function0 doesn't synchronize properly #9814

Closed
scabug opened this issue Jun 13, 2016 · 6 comments
Closed

Function0 doesn't synchronize properly #9814

scabug opened this issue Jun 13, 2016 · 6 comments
Assignees
@retronym
Labels
has PR specialization
Milestone
2.11.11

Comments

@scabug
Copy link

scabug commented Jun 13, 2016

I found that scalac infers that synchronized entire an entire method's code means that the method should be synchronized. On Function0#apply, this means that it synchronizes the apply method. However, it doesn't synchronize Function0#apply$mcV$sp, presumably thinking that folks will not call that method directly. However, scalac compiles function invocations down to call that method directly.

This means that methods which are marked synchronized are not really synchronized.

class Bar(foo: () => Unit) {
  def apply(): Unit = foo()
}

class Foo extends (() => Unit) {
  def apply(): Unit = synchronized {
    println("i'm synchronized!")
  }
}

javap -c Foo (edited for clarity)

...
  public synchronized void apply();
    Code:
       0: aload_0
       1: invokevirtual #64                 // Method apply$mcV$sp:()V
       4: return

  public void apply$mcV$sp();
    Code:
       0: getstatic     #70                 // Field scala/Predef$.MODULE$:Lscala/Predef$;
       3: ldc           #72                 // String i'm synchronized!
       5: invokevirtual #76                 // Method scala/Predef$.println:(Ljava/lang/Object;)V
       8: return

javap -c Bar (edited for clarity)

  public void apply();
    Code:
       0: aload_0
       1: getfield      #15                 // Field foo:Lscala/Function0;
       4: invokeinterface #20,  1           // InterfaceMethod scala/Function0.apply$mcV$sp:()V
       9: return
@scabug
Copy link
Author

scabug commented Jun 13, 2016

Imported From: https://issues.scala-lang.org/browse/SI-9814?orig=1
Reporter: Moses Nakamura (mosesn)
Affected Versions: 2.11.6
Other Milestones: 2.12.1

@scabug
Copy link
Author

scabug commented Jun 15, 2016

@retronym said:
Ouch! Thanks for the report.

A workaround is to restructure as:

  def apply(): Unit = {
    val self = this
    self.synchronized {
      println("i'm synchronized!")
    }
  }

The compiler looks for method bodies of the shape def foo(...) = this.synchronized { ... } and marks with a flag that will result ACC_SYNCHRONIZED in the bytecode. Other usages of synchronized (e.g. in my workaround) result in explicit bytecode for monitorenter / monitorexit.

https://github.com/scala/scala/blob/79e7334f93da4717ee846f74e48ab53533e6756d/src/compiler/scala/tools/nsc/transform/UnCurry.scala#L357-L371

Fixing this will involve something like:

diff --git a/src/compiler/scala/tools/nsc/transform/SpecializeTypes.scala b/src/compiler/scala/tools/nsc/transform/SpecializeTypes.scala
index 40ab8c0..dd47e62 100644
--- a/src/compiler/scala/tools/nsc/transform/SpecializeTypes.scala
+++ b/src/compiler/scala/tools/nsc/transform/SpecializeTypes.scala
@@ -1662,6 +1662,7 @@ abstract class SpecializeTypes extends InfoTransform with TypingTransformers {
               val tree1 = addBody(ddef, target)
               (new ChangeOwnerTraverser(target, tree1.symbol))(tree1.rhs)
               debuglog("changed owners, now: " + tree1)
+              if (target.hasFlag(Flags.SYNCHRONIZED)) ddef.symbol.setFlag(Flags.SYNCHRONIZED)
               deriveDefDef(tree1)(transform)

             case SpecialOverload(original, env) =>

I'll flesh this out and submit a patch for 2.11.9 + 2.12.x in the next week.

@scabug
Copy link
Author

scabug commented Jun 15, 2016

Moses Nakamura (mosesn) said:
Good idea for the work-around, I'll try it out.

@scabug
Copy link
Author

scabug commented Aug 14, 2016

Moses Nakamura (mosesn) said:
@retronym have you had a chance to take a look at this again? Would love for this to make it in into 2.12 or 2.11.9.

@scabug
Copy link
Author

scabug commented Aug 23, 2016

Moses Nakamura (mosesn) said:
@seth, any chance we can bump this to 2.12.0? this is a pretty unpleasant problem.

@scabug
Copy link
Author

scabug commented Nov 18, 2016

@retronym said:
scala/scala#5540

@scabug scabug closed this as completed Nov 25, 2016
@scabug scabug added this to the 2.11.9 milestone Apr 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@retronym retronym

Labels
has PR specialization
Projects
None yet
Milestone
2.11.11
Development

No branches or pull requests
2 participants
@retronym @scabug