google / guava

Google core libraries for Java
Apache License 2.0
50.11k stars 10.88k forks source link

Locks in try-with-resource #1608

Open gissuebot opened 9 years ago

gissuebot commented 9 years ago

Original issue created by kofemann on 2013-12-11 at 11:10 AM


Hi,

With java 7 we got try-with-resource semantics. Unfortunately, it does not work with locks and we still need to write

 lock.lock();  try {   //do some staff  } finally {     lock.unlock();  }

and it's quite often, finally block is forgotten.

There is a simple solution to that which nicely fits into guava

Lock lock = ...; try (AutoLock l = withLock(lock)) {      // access the locked resource }

The class including test attached.

-Kofemann.

gissuebot commented 9 years ago

Original comment posted by tomas.za...@intelis.cz on 2013-12-11 at 03:35 PM


OT: I am very curious about Guava team's response. According to links at http://stackoverflow.com/questions/16574353/any-risk-in-a-autocloseable-wrapper-for-java-util-concurrent-locks-lock , it seems the suggestion for adding this feature to Java was rejected.

gissuebot commented 9 years ago

Original comment posted by cgdecker@google.com on 2013-12-11 at 05:51 PM


FYI, Guava is still a JDK6 library and can't use AutoCloseable yet.

There's been some debate about this internally, which I'll try to sum up here:

  1. Try-with-resources was designed to solve resource management, for example opening and closing OutputStreams. This is actually VERY HARD (and verbose) to do correctly without try-with-resources. By comparison, acquiring and releasing locks is very, very simple. Releasing a lock cannot throw an exception, unlike closing an OutputStream.
  2. Because of what try-with-resources was designed for, it's extremely awkward to use for things like locks and "scoping" blocks to certain contexts (for example, a database transaction), because you're required to assign something to a variable in the resource declaration part of the try-with-resources block, but in neither of those cases do you actually need to USE the variable you create. This leads to both unused variable warnings as well as special try-with-resources warnings for unused resources.
  3. Despite that, people do seem to REALLY want to use try-with-resources with locks. And while it's unfortunately awkward to do, it does seem mostly harmless. Particularly since it's actually possible to do it in a way that doesn't create a new object each time you acquire a lock*.

Personally, while I would have been happy to recommend doing this had try-with-resources been designed in a way that intentionally supported this type of use, I don't really recommend it given the reality of the situation. I don't want to strongly discourage it either... that seems like a bit of a losing battle from what I can tell. I would suggest implementing it in a way that avoids object creation though.

class FooLock implements Lock {   private final Lock delegate;   private final Unlocker unlocker; // implements AutoCloseable

  FooLock(Lock delegate) {     this.delegate = delegate;     this.unlocker = new Unlocker(delegate);   }

  public Unlocker acquireLock() {     lock.lock();     return unlocker;   }

  // ... other methods }


Labels: Package-Concurrent

gissuebot commented 9 years ago

Original comment posted by kofemann on 2013-12-11 at 07:41 PM


Just want to comment on point 3.

I believe, the main motivation is simple syntax which we already have with synchronized:

synchronized(this){   ... }

It's natural, with the language which supports block to release resources allocated when leaving the block.

It's clear, that you shold not add into guava any crap ( that's the reason why we love it). So feel free to close this issue.

gissuebot commented 9 years ago

Original comment posted by cgdecker@google.com on 2013-12-11 at 08:05 PM


I agree that it's natural to want to be able to something similar to a synchronized block with locks... the problem being try-with-resources just doesn't really support that, no matter how much one might want it to.

I'm not sure I want to close this issue just yet. It's not something we can do in Guava at the moment as a JDK6 library, but I don't want to rule it out entirely, since given the huge desire to do this it might be preferable to create one good implementation in Guava. Maybe.


Status: Research

cpovirk commented 6 years ago

Internally, bug 112137598 shows a large number of places where people are omitting the try-finally or doing it wrong. Maybe it's time for us to revisit this now that we have a Java 8 branch?

cpovirk commented 6 years ago

(though there are still legitimate reservations here, and we can question whether the helper would actually have helped people avoid bugs)

jbduncan commented 6 years ago

Maybe it's time for us to revisit this now that we have a Java 8 branch?

@cpovirk Do you know if the Android branch supports try-with-resource as well?

If it does, then is there a reason why we'd not want the proposed withLock or similar to be available in guava-android as well?

cpovirk commented 6 years ago

Good question. I am seeing contradictory information about whether it would work.

"In addition to the Java 8 language features and APIs above, Android Studio 3.0 and later extends support for try-with-resources to all Android API levels."

However, it's not 100% clear to me whether arbitrary use of AutoCloseable works, since it's labeled "adding in API level 19". But then I see claims that it's secretly been available since Ice Cream Sandwich, which is what we test with.

jbduncan commented 6 years ago

Hmm, that is rather confusing. :thinking:

I'm also a bit confused by the claim that "Android Studio [...] extends support for try-with-resources". Does one have to specifically use Android Studio to be able to use try-with-resources, or does one only need the Android SDK (e.g. one that supports API level 19+) and some appropriate Gradle setup?

My knowledge of Android development is a bit limited (the last time I worked with it was when I did a university module a few years ago, when Eclipse was still the official IDE), so I don't know if Android Studio is necessary for Android development nowadays or if one can still get away with command-line tools for some/all tasks.

JakeWharton commented 6 years ago

You left off "to all Android API levels" from your quote which provides much needed context as to the why. It's not supported on all API levels, and 3.0 provides that functionality.

jbduncan commented 6 years ago

@JakeWharton Interesting. So Android Studio itself adds a step to the mix that allows one to use try-with-resources on all API levels? I'm curious to understand how that works, as by my limited understanding Android Studio delegates build tasks to Gradle. :)

JakeWharton commented 6 years ago

It has nothing to do with the IDE, all of the functionality is in the build tooling. In 3.0 and 3.1 it was provided with a Java bytecode transformer called desugar which was lifted from Bazel. In 3.2 it's implemented natively in the D8 compiler (of Java bytecode to Dalvik bytecode).

jbduncan commented 6 years ago

Oh, I see now! Cool, thanks for clarifying things for me @JakeWharton. :+1:

cpovirk commented 6 years ago

Let's consider the remainder of #2853, which proposes a similar API for Monitor, to be part of this issue.

jsampson commented 4 years ago

In Java 8+, we can do even better with lambdas rather than try-with-resources. Imagine something like this:

public static void runWithLock(Lock lock, Runnable task) {
  lock.lock();
  try {
    task.run();
  } finally {
    lock.unlock();
  }
}

Such a method would be used like so:

runWithLock(lock, () -> {
  // do some stuff
});

A full solution would include overloads for interruptibility, timeouts, etc., but this is a basic start.

More radically, we can imagine a whole new Lock replacement designed around lambdas. I have proposed one here: https://github.com/google/guava/issues/3265