search

yf0994 / guava-libraries

Automatically exported from code.google.com/p/guava-libraries
Apache License 2.0
0 stars 0 forks source link

LoadingCache doesn't get along well with interrupted threads #1122

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
two separate problems here:

1) When a thread is interrupted while blocking on another thread's cache load, 
it swallows the interrupt and continues to block.  This makes no sense and is 
not best practice; Future.get() throws InterruptedException for a reason.

2) When a CacheLoader is interrupted and throws InterruptedException and/or 
leaves the interrupted status set while throwing another exception (we tend to 
turn it into our own Runtime InterruptedException), that exception is 
propagated to any other thread blocking on the cache load instead of letting 
one of them attempt to load.

I think these are both mistakes.

Original issue reported on code.google.com by dterr...@gmail.com on 28 Aug 2012 at 6:20

GoogleCodeExporter commented 9 years ago 
1) I'm resistant to the idea that a method is obligated to be interruptible 
just because it takes a long time or calls wait() somewhere, but I'm 
sympathetic to throwing IE here.  Given my mixed feelings, I know I discussed 
this at some point, but I can't find record of this in my email, so it may have 
happened in person or in some other format.  Anyway, I think it came down to 
making LoadingCache as transparent as possible.  If a thread makes the first 
call to get() for a key, then interruption will have no effect.  In some cases, 
in fact, it's known that only one thread will ever make calls to get(), in 
which case "throws InterruptedException" would be entirely bogus.  When 
multiple threads are calling get(), it's odd for interruption to sometimes 
interrupt the call and sometimes not.  This isn't clearly the right decision, 
but it's what we went with.  If it's any consolation, the future 
AsyncLoadingCache will provide a getFuture() method, and Future interruption 
will work properly.

2) Yes.  We have an internal bug filed for this, but we haven't done any work 
to fix it.  The cache code is complex enough that any significant change is 
risky, so we've been waiting to hear from someone who encountered this in the 
wild.  Have you?  Again, the future AsyncLoadingCache will work better.

Original comment by cpov...@google.com on 28 Aug 2012 at 6:37

GoogleCodeExporter commented 9 years ago 
I'm personally very supportive of continuing to suppress exceptions on 
getUnchecked, but I can sympathize with the desire to be able to interrupt a 
cache load.

Original comment by wasserman.louis on 28 Aug 2012 at 10:48

GoogleCodeExporter commented 9 years ago 
I'd be ok with throwing (a hypothetical) RuntimeInterruptedException from 
getUnchecked(), but it's not just getUnchecked(), get() also blocks forever.

Original comment by dterr...@gmail.com on 6 Sep 2012 at 6:14

GoogleCodeExporter commented 9 years ago 
Somehow we have no documentation for this :\  I'm accepting this issue for at 
least the purpose of adding documentation.  Thanks.

Original comment by cpov...@google.com on 6 Sep 2012 at 6:47

GoogleCodeExporter commented 9 years ago

Original comment by cpov...@google.com on 6 Sep 2012 at 6:48

GoogleCodeExporter commented 9 years ago

Original comment by kevinb@google.com on 15 Oct 2012 at 8:24

GoogleCodeExporter commented 9 years ago

Original comment by kak@google.com on 23 Oct 2012 at 4:48

GoogleCodeExporter commented 9 years ago

Original comment by wasserman.louis on 29 Jan 2013 at 6:47

GoogleCodeExporter commented 9 years ago

Original comment by kevinb@google.com on 12 Mar 2013 at 6:43

GoogleCodeExporter commented 9 years ago 
I've written this up in a little more detail at 
<http://code.google.com/p/guava-libraries/wiki/CachesExplained#Interruption>. I 
don't think there's anything new to you there, but I'm hoping it's a gentler 
introduction for those who hadn't already identified the problem themselves.

I've submitted a CL internally (to be mirrored out soon) that links from the 
LoadingCache Javadocs to that Wiki page.

We may yet do something about problem #2, but that's beyond my capabilities at 
this point.

Original comment by cpov...@google.com on 13 Mar 2013 at 3:06

GoogleCodeExporter commented 9 years ago 
I was working on the functionality of an "AsyncLoadingCache" when I stumbled 
across this issue (which seems to be the only mention of this future class). I 
think I've found a neat workaround for this issue and the AsyncLoadingCache 
itself: By putting an ExecutorService (could be a SameThreadExecutorService!) 
inside a CacheLoader<K, Future<V>> implementation (see 
https://gist.github.com/sfussenegger/5212107 for details). However, it's that 
simple that I'm wondering if I'm missing the whole point here :) Could you 
please comment?

Original comment by s...@molindo.at on 21 Mar 2013 at 10:44

GoogleCodeExporter commented 9 years ago 
I realized that I no longer remembered what the point was, so I did some 
research :) I've filed issue 1350 with some of my results. It can serve as a 
forum for general AsyncLoadingCache discussions. As you'll see there, 
LoadingCache<K, Future<V>> works quite well in the simple cases and less well 
thereafter.

Original comment by cpov...@google.com on 21 Mar 2013 at 3:25

GoogleCodeExporter commented 9 years ago 
This issue has been migrated to GitHub.

It can be found at https://github.com/google/guava/issues/<id>

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:13

GoogleCodeExporter commented 9 years ago

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:18

GoogleCodeExporter commented 9 years ago

Original comment by cgdecker@google.com on 3 Nov 2014 at 9:08