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

Compiler warnings do not appear to recognize system interactions #8400

Closed
scabug opened this issue Mar 12, 2014 · 6 comments
Closed

Compiler warnings do not appear to recognize system interactions #8400

scabug opened this issue Mar 12, 2014 · 6 comments

Comments

@scabug
Copy link

scabug commented Mar 12, 2014

The following is Test.scala

object Test {

private val shutdownHook = sys.addShutdownHook(shutdown())

def main(args: Array[String]) {
(1 to 100) foreach {i => println(i); Thread.sleep(1000)}
}

def shutdown() {
println("Shutting down.")
}

}

Running the follwing returns:
$ scalac -Ywarn-unused Test.scala
Test.scala:3: warning: private val in object Test is never used
private val shutdownHook = sys.addShutdownHook(shutdown())
^

However, letting the program run to completion prints out "Shutting down" as does killing the program with ^C and ^Z.

Further, the following code compiled with the above flag now has two warnings.

object Test2 {

private val shutdownHook = sys.addShutdownHook(shutdown())
private var bool = true

def main(args: Array[String]) {
(1 to 100) foreach {i => println(i); Thread.sleep(1000)}
}

def shutdown() {
bool = false
println("Shutting down.")
}

}

$scalac -Ywarn-unused Test2.scala
Test.scala:3: warning: private val in object Test2 is never used
private val shutdownHook = sys.addShutdownHook(shutdown())
^
Test.scala:4: warning: private var in object Test2 is never used
private var bool = true
^

Perhaps this is me misunderstanding how the compiler works with the JVM, but it feels wrong nonetheless.

@scabug
Copy link
Author

scabug commented Mar 12, 2014

Imported From: https://issues.scala-lang.org/browse/SI-8400?orig=1
Reporter: Eric Hartsuyker (ehartsuyker)
Affected Versions: 2.11.0-RC1

@scabug
Copy link
Author

scabug commented Mar 12, 2014

@som-snytt said:
I think the diagnosis is correct, that there is a misunderstanding.

The warning is just saying that the variable shutdownHook is not read. Maybe you forgot to do something useful with it:

      if (i > 50) { Console println s"Unhooking: ${shutdownHook.remove()}" ; sys exit 0 }

The excellent ScalaDoc notes: "The hook is automatically registered: the returned value can be ignored." That is, you don't need to hold the reference.

@scabug
Copy link
Author

scabug commented Mar 12, 2014

@adriaanm said:
Not strictly a bug -- happy to improve the wording of the warning message if you have a suggestion.

@scabug scabug closed this as completed Mar 12, 2014
@scabug
Copy link
Author

scabug commented Mar 13, 2014

Eric Hartsuyker (ehartsuyker) said:
If this is not a bug, then it could be a feature improvement for the compiler to not warn against it. The following will properly catch a ^C and shutdown in a way that allows all the futures to finish. I feel that this is useful and not an "unused val." Yet compiling warns against it.

object Test {

  import scala.concurrent.Future
  import scala.concurrent.ExecutionContext.Implicits.global
  import scala.collection.mutable

  private val shutdownHook = sys.addShutdownHook(shutdown())
  private var shouldRun = true
  private var workers = new mutable.HashSet[Future[Long]]
  private var lastCompletedWorker = 0L
  private lazy val SYSTEM_SHUTDOWN_TIMEOUT = 3000L

  def main(args: Array[String]) {
    receive()
  }

  private def receive(): Unit = {

    workers += Future {
      Thread.sleep(1000)
      val t = System.currentTimeMillis
      println(t)
      t
    }
    println("Workers: " + workers.size)
    lazy val completed = workers.filter(_.isCompleted)

    if (completed.size > 0) lastCompletedWorker = System.currentTimeMillis

    workers --= completed
    Thread.sleep(100)
    if (shouldRun) receive()
  }

  private def shutdown() {
    println("Shutting down")
    shouldRun = false
    while (workers.size > 0 && System.currentTimeMillis - lastCompletedWorker < SYSTEM_SHUTDOWN_TIMEOUT) {
      // do nothing
    }
    println("Workers done")
  }

}

@scabug
Copy link
Author

scabug commented Mar 13, 2014

@som-snytt said (edited on Mar 13, 2014 6:10:58 PM UTC):
Aside from the lack of thread safety, the example works without the private member, which is in fact unused in the code, like the warning says. You can add the hook without the assignment.

But if you did need to hold a reference (to avoid garbage collection), it would be nice to have a way to turn off the warning; see other tix about turning off warnings, or let's call it "tuning" warnings.

Besides the thread safety between the hook and receive, there's also a race because you set the hook before initializing everything else. (In an object, with the init run from static block, maybe there's a guarantee about the hook not running?)

@scabug
Copy link
Author

scabug commented Mar 13, 2014

Eric Hartsuyker (ehartsuyker) said:
I'll look into that to see if I'm implementing it correctly. Thank you.

@scabug scabug added the quickfix label Apr 7, 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

1 participant