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

Allow Using or scopeExit in plain functions #253

Open
wants to merge 7 commits into
base: 1.5.x
Choose a base branch
from

Conversation

lxohi
Copy link

@lxohi lxohi commented Jun 1, 2019

Defer keyword works likes defer in Golang.

It runs the deferred statements in the reversed sequence when current function finished.

@Atry
Copy link
Collaborator

Atry commented Jun 1, 2019 via email

@lxohi
Copy link
Author

lxohi commented Jun 2, 2019

Using side:

  • Execute the enclosed op immediately.
  • The op needs to return AutoCloseable.
  • Call the AutoCloseable.close() at the end of current function.

Defer side:

  • Execute the enclosed op at the end of current function.
  • The op have no need to return anything.

We can let Using do the similar thing by adding a implicit conversion like this:

  implicit def toAutoCloseable(op: => Unit): () => AutoCloseable = () => {
    new AutoCloseable {
      override def close(): Unit = op
    }
  }

  private def tryUsingToDefer() = Task {
    !Using(println("1"))
    !Using(println("2"))
    !Using(println("3"))
    println("hello world")
  }

But I don't think it is a good practice because:

  • The behavior of that implicit conversion looks a little bit weird.
  • This may cause confusion with !Using lines. Adding this implicit conversion will cause the meaning of Using keyword becomes if you put a AutoCloseable in the parentheses it will do A otherwise it will do B.

And by the way the current implementation of Defer is allowed to be used in a normal function block.

@Atry
Copy link
Collaborator

Atry commented Jun 2, 2019 via email

@lxohi
Copy link
Author

lxohi commented Jun 2, 2019

I've tried using SAM types in Scala 2.11 with modified code like this:

  private def tryUsingToDefer() = Task {
    !Using(() => println("1"))
    !Using(() => println("2"))
    !Using(() => println("3"))
    println("hello world")
  }

It just not compile.

I think this is because the parameter type of Using is not AutoCloseable but something have type bounds of AutoCloseable and it makes SAM types doesn't works in this case.

@Atry
Copy link
Collaborator

Atry commented Jun 3, 2019

Yes, you are right. We need the following method to enable SAM types in Scala 2.12+.

object Using {
  def apply[R <: AutoCloseable](r: => R)(
      implicit dummyImplicit: DummyImplicit = DummyImplicit.dummyImplicit): Using[R] = new Using(r _)
  def apply(r: => AutoCloseable)(implicit dummyImplicit: DummyImplicit, dummyImplicit2: DummyImplicit) = new Using(() => r)
}

@Atry
Copy link
Collaborator

Atry commented Jun 3, 2019

I have some thoughts about the Defer keyword:

  1. The current Using implementation is very sophisticated for exception handling. Is it really worth to duplicate the implementation?

  2. Should we allow for Defer / Using in plain functions? I worry about possible ambiguity. For example, what is the expected behavior of Using in the following example?

    import akka.http.scaladsl.server.Directives._
    import com.thoughtworks.dsl.keywords.Using
    import java.nio.file.Files
    import java.nio.file.Paths
    import scala.collection.JavaConverters._
    
    def currentDirectoryRoute = {
      pathPrefix("current-directory") {
        get {
          parameters("glob") { glob =>
            val currentDirectory = !Using(Files.newDirectoryStream(Paths.get(""), glob))
            path("file-names") {
              complete(currentDirectory.iterator.asScala.map(_.toString).mkString(","))
            } ~ path("number-of-files") {
              complete(currentDirectory.iterator.asScala.size.toString)
            }
          }
        }
      }
    }

    The above code will close currentDirectory after the route is complete asynchronously at the moment, because Using supports only asynchronous domains. If we allow plain function domains, will currentDirectory be closed before the asynchronous Future is complete?

  3. Is it important to support a short syntax for Scala 2.11?

@lxohi
Copy link
Author

lxohi commented Jun 3, 2019

Thank you for your detailed reply!

About the points metioned above:

  1. Not really. With SAM types and if Using can be used in normal functions, I think it's totally fine.

  2. The case you pointed out would be a bad case like Scala's return keyword and I believe it is not acceptable.
    But I've tested that Defer keyword was executed when the nearest function finished.

    My test code is:

      def doOp(op: () => Future[Unit]): Unit = {
        import scala.concurrent.duration._
        scala.concurrent.Await.result(op(), 1.minute)
      }
    
      def functionInsideMethod(): Unit = {
        doOp { () =>
          Future {
            !Defer(println(Thread.currentThread().getName, "deferred"))
            println(Thread.currentThread().getName, "function body")
          }
        }
        println(Thread.currentThread().getName, "method body")
      }

    And it prints:

    (default-akka.actor.default-dispatcher-5,function body)
    (default-akka.actor.default-dispatcher-2,deferred)
    (main,method body)
    

    It seems works as expected (like the Golang one).
    I'm not sure is there any corner cases or flaws will let Using/Defer behaves like return in normal functions?

  3. By adding the new apply method, SAM types will works in Scala 2.11 :)

@Atry Atry changed the title Add Defer keyword Allow Using or defer in plain functions Jun 3, 2019
@Atry
Copy link
Collaborator

Atry commented Jun 3, 2019

Not really. With SAM types and if Using can be used in normal functions, I think it's totally fine.

OK. Since Using covers all the use case of Defer, let's split this PR into two issues:

@lxohi
Copy link
Author

lxohi commented Jun 4, 2019

Will modify this branch as soon as #255 was merged (>ω<)

@Atry Atry changed the title Allow Using or defer in plain functions Allow Using or scopeExit in plain functions Jun 4, 2019
@lxohi
Copy link
Author

lxohi commented Jun 5, 2019

Hi, because I'm gonna disappear for 4 days, here's just a quick update.

I've modified code which contains plain function support for Using with several test cases.

While scopeExit works fine in function literals like the sample code above, it executed in the wrong scope when passing to a call-by-name param.

Here is a test case will fail due to this issue.

I think this is because call-by-name param does not treated as domain. And this is caused by it's representation is a normal param in the AST not a function.

I'm currently not sure is it doable/a good idea to let the compiler treat call-by-name param as a domain. I will try it when I'm back.

@Atry
Copy link
Collaborator

Atry commented Jun 5, 2019

@lxohi We can discuss problem around call-by-name parameters at #22

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

Successfully merging this pull request may close these issues.

None yet

2 participants