Should shutdownAndAwaitTermination and awaitUninterruptibly be @CheckReturnValue?

[cpovirk]

(For that matter, ExecutorService.awaitTermination itself -- though that change would ideally be made in Error Prone itself, rather than in our subclass ListeningExecutorService.)

I just had a bug in my test because I was thinking that shutdownAndAwaitTermination would throw an exception if the executor didn't terminate in time. I'm not sure that's what all callers would want, but it does seem weird to specifically wait around for termination but wait only so long. We could prompt users to think about this by adding @CheckReturnValue (or really "by removing @CanIgnoreReturnValue").

[cpovirk]

OK, maybe it's not that weird: If I'm writing a command-line application, I might want to wait n seconds for background work to finish up. If it's not done by then, then I want to exit anyway. Still, I would probably want to know that the background work is still underway so that I can warn the user about it in case it was important. (If it wasn't important, after all, then why would I wait at all?) So I'd still want to check the return value.

I see some shutdownAndAwaitTermination calls in test teardown. I wonder if a lot of them are just making tests slower for no reason (vs. just calling shutdown -- or shutdownNow -- and walking away from the explosions). Again, if the background work was doing anything important, then I'd probably want to check if it's completed (and probably even check that it completed successfully by looking at some of the returned Future instances).

Really, I have the same concern about additional concurrency methods like CountDownLatch.await(long, TimeUnit) and our Uninterruptibles wrappers of them (so far, just that one method). I see a bunch of bare await(long, TimeUnit) calls in our tests, and I wonder if many of them should be wrapped with assertTrue. I see that I have a TODO about this.

[cpovirk]

(Maybe some tests don't want assertTrue in teardown because it might hide the "real" failure in a test? I forget how failures in teardown work. Ideally they'd be suppressed exceptions, but I suspect that's not how it works?)

[update that I forgot to link to this issue: https://github.com/google/guava/commit/a183b59ace3dc778db40c763666c0aaf61c81f77 was another step in this direction.]