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

Add scala.util.Using, for automatic resource management #6907

Merged
merged 7 commits into from Aug 15, 2018

Conversation

@NthPortal
Contributor

NthPortal commented Jul 9, 2018

Resolves scala/bug#11003
Resolves scala/bug#8028 (I think)
(and possibly others)

@NthPortal NthPortal added the WIP label Jul 9, 2018

@scala-jenkins scala-jenkins added this to the 2.13.0-M5 milestone Jul 9, 2018

@NthPortal

This comment has been minimized.

Contributor

NthPortal commented Jul 9, 2018

bikeshedding welcome

@NthPortal NthPortal force-pushed the NthPortal:bug#11003/PR branch from cc8d3c4 to 6323af0 Jul 9, 2018

@Ichoran

This comment has been minimized.

Contributor

Ichoran commented Jul 9, 2018

I would think the signature should be [R: Resource, A](=>R)(R => A): Try[A]

@hrhino

This comment has been minimized.

Member

hrhino commented Jul 9, 2018

See also discussion at #6347.

@NthPortal

This comment has been minimized.

Contributor

NthPortal commented Jul 9, 2018

@Ichoran I guess I have two comments on that

  • why => R? Is there an advantage to having its evaluation be deferred?
  • there are a few disadvantages to having it return a Try
    • it becomes impossible to not use a Try; on the other hand, as it is, you can just wrap the whole thing in a Try if you want that
    • it makes it uglier to nest/compose using blocks; if you have four resources, you need to create four Trys, which really isn't necessary for the computation

regarding the latter point, this is what the implementation of the four resource method would look like if it returns a Try

using(resource1) { r1 =>
  using(resource2) { r2 =>
    using(resource3) { r3 =>
      using(resource4) { r4 =>
        body(r1, r2, r3, r4)
      }.get
    }.get
  }.get
}

not horrible, but certainly uglier

@Ichoran

This comment has been minimized.

Contributor

Ichoran commented Jul 9, 2018

You want Try because throwing exceptions all over the place is messy. Maybe you should have two versions, one that wraps the exceptions and one that doesn't, but the usual case should be to use the Try version since you can't muck that one up.

If you're going to have a Try variant, then you need => R so that resource creation exceptions are also handled cleanly.

And of course you ought to be able to

for {
  r1 <- using(resource1)
  r2 <- using(resource2)
  r3 <- using(resource3)
  r4 <- using(resource4)
} yield body(r1, r2, r3, r4)

which means the whole thing should be an applicative functor, or functor-like (I don't know what the non-identity version is, where map is (F[A], A => B) => G[B] and flatMap is (F[A], A =>G[B]) => G[B]) for a specific permitted G, e.g. Try[A] or Either[ResourceException, A].

@Ichoran

This comment has been minimized.

Contributor

Ichoran commented Jul 9, 2018

Okay, this all works. I just have a stub implementation for the actual resource logic, but I envision it like this:

class Using[R](r: => R) {
  import scala.util.{Try, Success, Failure}
  def apply[A](f: R => A)(implicit ev: Using.Resource[R]): Try[A] = map[A](f)(ev)
  def map[A](f: R => A)(implicit ev: Using.Resource[R]): Try[A] = Try{ val myR = r; val ans = f(myR); ev.close(myR); ans }
  def flatMap[A](f: R => Try[A])(implicit ev: Using.Resource[R]): Try[A] = Try{ val myR = r; val ans = f(myR); ev.close(myR); ans }.flatten
}
object Using {
  trait Resource[R]{ def close(r: R): Unit }
  def apply[R](r: => R): Using[R] = new Using(r)
}

Then we have usage like so:

case class Id[A](value: A) {}
object Id { 
  implicit def resourcedId[A]: Using.Resource[Id[A]] = new Using.Resource[Id[A]] { def close(r: Id[A]) = () } 
} 

@ for { u <- Using(Id(2)); v <- Using(Id("salmon")) } yield v.value * u.value 
res2: scala.util.Try[String] = Success("salmonsalmon")

@ 
Using(Id(true)).flatMap{ bool => Using(Id('x')){ c => if (bool.value) c.value.toUpper else c }} 
res5: scala.util.Try[Any] = Success('X')
@Ichoran

This comment has been minimized.

Contributor

Ichoran commented Jul 9, 2018

I'm not sure what the name of this not-exactly-a-Monad thing is, if it has one, but the usage is really nice. You just use your stuff, and the right thing happens, and exceptions are caught.

@NthPortal

This comment has been minimized.

Contributor

NthPortal commented Jul 9, 2018

@Ichoran my only concern is how fatal exceptions are handled. I personally think that even in the case of a fatal exception, we ought to attempt to close the resource before re-throwing the exception (especially if it's a NonLocalReturnControl). I will have to have a look at your implementation to see how it handles it.

I think it makes sense to have both, but with the Try one more prominently defined, as you suggest. If we're to have a Using class though, should the throwing methods be renamed? Or is there not too much of a worry of having them be called, since it would require importing the entire contents of object Using?

Also, I would be more inclined to put the implicit Resource in the constructor of a Using, rather than on each of the methods.

@smarter

This comment has been minimized.

Contributor

smarter commented Jul 9, 2018

@NthPortal

This comment has been minimized.

Contributor

NthPortal commented Jul 9, 2018

@Ichoran pushed the monad-ish impl you suggested. I'm open to burying the raw methods a bit more to make them less obvious

@Ichoran

This comment has been minimized.

Contributor

Ichoran commented Jul 9, 2018

@NthPortal - You can put the typeclasses on the constructor, but then you can't chain the applies. So it would have to be Using(file.open).map(_.slurp) not Using(file.open){ _.slurp } (assuming that there was an open method on file, and that you can slurp an OpenFile).

* @return the result of the operation, if neither the operation nor
* closing the resource throws
*/
def using[R: Resource, A](resource: R)(body: R => A): A = {

This comment has been minimized.

@Ichoran

Ichoran Jul 9, 2018

Contributor

I don't think Using.using is good nomenclature. Maybe Using.direct or Using.unsafe or Using.unwrapped?

This comment has been minimized.

@NthPortal

NthPortal Jul 9, 2018

Contributor

my only concern is that I'd prefer a method name that, if imported statically, still makes sense.

e.g. Using.direct(file.open) makes sense, but direct(file.open) doesn't really

This comment has been minimized.

@Ichoran

Ichoran Jul 9, 2018

Contributor

withResource?

This comment has been minimized.

@NthPortal

NthPortal Jul 14, 2018

Contributor

that more I play with it in my mind, the less happy I am with the name withResource. something about it doesn't flow right.

what do you think of just resource or tryWithResource? The former works well when not imported - Using.resource.

This comment has been minimized.

@NthPortal

NthPortal Jul 14, 2018

Contributor

or another possibility: tryWith

This comment has been minimized.

@NthPortal

NthPortal Jul 14, 2018

Contributor

(for comparison)

Using.withResource(new FileInputStream("foo.txt")) { _ => () }
withResource(new FileInputStream("foo.txt")) { _ => () }

Using.resource(new FileInputStream("foo.txt")) { _ => () }
resource(new FileInputStream("foo.txt")) { _ => () }

Using.tryWithResource(new FileInputStream("foo.txt")) { _ => () }
tryWithResource(new FileInputStream("foo.txt")) { _ => () }

Using.tryWith(new FileInputStream("foo.txt")) { _ => () }
tryWith(new FileInputStream("foo.txt")) { _ => () }

@NthPortal NthPortal changed the title from [WIP] Add utility for automatic resource management to [WIP] Add utility for automatic resource management [ci: last-only] Jul 9, 2018

@NthPortal NthPortal removed the WIP label Jul 14, 2018

@NthPortal

This comment has been minimized.

Contributor

NthPortal commented Jul 14, 2018

I think the implementation is pretty much done at this point. The scaladoc could use some improvement, which I would appreciate input on. The implementation is ready for review.

@NthPortal

This comment has been minimized.

Contributor

NthPortal commented Jul 14, 2018

should this be in scala.util.control (as it is currently) or in scala.util where Try is?

@NthPortal NthPortal changed the title from [WIP] Add utility for automatic resource management [ci: last-only] to Add utility for automatic resource management Jul 14, 2018

@NthPortal NthPortal changed the title from Add utility for automatic resource management to Add utility for automatic resource management [ci: last-only] Jul 14, 2018

@NthPortal

This comment has been minimized.

Contributor

NthPortal commented Jul 20, 2018

review @kmizu

@NthPortal NthPortal force-pushed the NthPortal:bug#11003/PR branch from da33c76 to 19d5adf Jul 24, 2018

@NthPortal NthPortal changed the title from Add utility for automatic resource management [ci: last-only] to Add utility for automatic resource management Jul 24, 2018

* @param f the operation to perform
* @param r an implicit [[Using.Resource]]
* @tparam A the return type of the operation
* @throws java.lang.IllegalStateException if the resource has already been used

This comment has been minimized.

@NthPortal

NthPortal Jul 27, 2018

Contributor

I am open to being convinced that the resource can be generated again using the by-name parameter, and that the Using instance can be used arbitrarily many times.

This comment has been minimized.

@kmizu

kmizu Aug 1, 2018

Contributor

@NthPortal Sorry. I forgot to review.

This comment has been minimized.

@kmizu

kmizu Aug 13, 2018

Contributor

@NthPortal It looks good to me as far as I can see

@dwijnand

This comment has been minimized.

Member

dwijnand commented Jul 27, 2018

should this be in scala.util.control (as it is currently) or in scala.util where Try is?

JFYI I noticed it's declared in scala.util.control but defined in scala/util/Using.scala.

@NthPortal

This comment has been minimized.

Contributor

NthPortal commented Jul 27, 2018

Good catch!

I expected IntelliJ to automatically change the package statement when I moved the files, but apparently it didn't ¯\_(ツ)_/¯

I'll fix that

Add Using for resource management
Add Using utility to perform automatic resource management.
@NthPortal

This comment has been minimized.

Contributor

NthPortal commented Aug 9, 2018

@joshlemer bonus side effect of simplifying the implicit Resource for AutoClosable!

@joshlemer

This comment has been minimized.

Contributor

joshlemer commented Aug 9, 2018

I am just throwing out a suggestion but maybe a factory method

Resource.fromTry[A](f: A => Try[_]): Resource[A]

might come frequently in handy as well

EDIT: Whoops fixed typo in the code

@NthPortal

This comment has been minimized.

Contributor

NthPortal commented Aug 9, 2018

@joshlemer It's not immediately apparent to me what that does

@joshlemer

This comment has been minimized.

Contributor

joshlemer commented Aug 9, 2018

@NthPortal something like

def fromTry[A](f: A => Try[_]): Resource[A] = (a: A) => f(a).failed.foreach(throw _)
@NthPortal

This comment has been minimized.

Contributor

NthPortal commented Aug 9, 2018

@joshlemer what's the value of catching exceptions in a Try just to rethrow them immediately?

@joshlemer

This comment has been minimized.

Contributor

joshlemer commented Aug 9, 2018

@NthPortal I was thinking more in the case where a user has a library which returns a Try for some IO operation, rather than itself throwing. In that case, the user would have to implement a Resource[T] which just throws any failed Try's, so it might be handy to have that case covered.

@NthPortal

This comment has been minimized.

Contributor

NthPortal commented Aug 9, 2018

@joshlemer aha, I see. I'm not super into it, but I'd love to hear others' thoughts on it

@lrytz

Very nice, thanks! Asking for a few scaladoc improvements.

* operation using resources, after which it will release the resources, in reverse order
* of their creation. The resource opening, operation, and resource releasing are wrapped
* in a `Try`.
*

This comment has been minimized.

@lrytz

lrytz Aug 9, 2018

Member

I think the doc should explain what a resource is, how the Resource type class is used, what the user needs to do for non-AutoClosables.

This comment has been minimized.

@lrytz

lrytz Aug 9, 2018

Member

Since there are two ways of using Using, I think this comment (which is the entry-point) should mention both, say why they exist, what the differences are.

This comment has been minimized.

@NthPortal

NthPortal Aug 11, 2018

Contributor

When you say two ways, to you mean Using(foo){ _.bar } vs for (f <- Using(foo)) yield f.bar, or Using(foo){ _.bar } vs Using.resource(foo){ _.bar }?

This comment has been minimized.

@lrytz

lrytz Aug 13, 2018

Member

I meant Using(foo) vs Using.resource(foo), but explaining the possibilities how to use the monadic version is also welcome.

}
/** @define recommendUsing It is highly recommended to use the `Using` construct,
* which safely wraps resource usage and management in a `Try`.

This comment has been minimized.

@lrytz

lrytz Aug 9, 2018

Member

It's maybe not clear to everyone what the Using construct is

Show resolved Hide resolved src/library/scala/util/Using.scala
@joshlemer

This comment has been minimized.

Contributor

joshlemer commented Aug 9, 2018

@NthPortal is there a reason not to make Using covariant?

@NthPortal

This comment has been minimized.

Contributor

NthPortal commented Aug 9, 2018

@joshlemer not that I can think of, but I also can't think of a reason why you'd ever declare it as a parameter to something, so I don't think it makes a difference. If you want to use its result for something, just use a Try.

@SethTisue

This comment has been minimized.

Member

SethTisue commented Aug 9, 2018

but won't add new things in RC1). @SethTisue, I defer to you as M5 lead, but I've put it back on the M5 radar for now

maybe it could still make M5, but I think it could wait for RC1 if it must. we aren't adding new things post-M5, but I think we should allow exceptions for new things that were already PR'ed well in advance of the M5 deadline. especially something like this that isn't being used yet and has zero effect on code that doesn't use it.

@NthPortal

This comment has been minimized.

Contributor

NthPortal commented Aug 9, 2018

@SethTisue I think that's a good idea. I don't know if I can push through the doc changes by tomorrow while also trying to get LazyList done

* val lines: Try[List[String]] = Using(resource1) { r1 =>
* r1.lines.toList
* }
* }}}

This comment has been minimized.

@NthPortal

NthPortal Aug 12, 2018

Contributor

@Ichoran what are your thoughts on removing the apply method, so that there aren't two ways to use it - you have to use a for comprehension (or manual map/flatMap)?

This comment has been minimized.

@NthPortal

NthPortal Aug 12, 2018

Contributor

This has the added benefit of allowing us to pass the implicit Resource on construction

This comment has been minimized.

@Ichoran

Ichoran Aug 12, 2018

Contributor

The for-comprehension-only version probably ought to be called Use or Used, not Using, if there's only that option. I generally prefer the apply version, but I don't have a strong preference.

This comment has been minimized.

@NthPortal

NthPortal Aug 12, 2018

Contributor

I don't feel strongly about it either; I'm just concerned that some others might.

I'm going to leave it as is for now

@SethTisue

This comment has been minimized.

Member

SethTisue commented Aug 15, 2018

shall we merge as-is, to get feedback from a broader group, and figure there's still time to tweak it for RC1?

@lrytz

This comment has been minimized.

Member

lrytz commented Aug 15, 2018

That works for me!

@SethTisue SethTisue merged commit 05f9d9b into scala:2.13.x Aug 15, 2018

4 checks passed

cla @NthPortal signed the Scala CLA. Thanks!
Details
combined All previous commits successful.
continuous-integration/travis-ci/pr The Travis CI build passed
Details
validate-main [3702] SUCCESS. Took 41 min.
Details
@NthPortal

This comment has been minimized.

Contributor

NthPortal commented Aug 15, 2018

(this probably should have been squashed first, oops)

@SethTisue

This comment has been minimized.

Member

SethTisue commented Aug 15, 2018

doh, my bad!

@SethTisue SethTisue changed the title from Add utility for automatic resource management to Add scala.util.Using, for automatic resource management Aug 22, 2018

@NthPortal NthPortal deleted the NthPortal:bug#11003/PR branch Sep 2, 2018

@julienrf julienrf referenced this pull request Dec 10, 2018

Open

On using `Using` #592

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment