context: Make CancellableContext implement Closeable

[zpencer]

This achieves two things:

• communicates that CancellableContext is a resource that must be freed
• provides the ability to use the try-with-resources idiom to cancel the context when we finish up some blocking work. For users, this may be a better API than runAndCancel and callAndCancel.

@ejona86 and I considered adding a try-with-resources API that attaches the CancellableContext before the try, and both cancels as well as detaches at the close. But the problem is that the close runs before any catch statements, so users will not be able to use the exception as the cancellation cause unless they use a nested try/catch.

We also considered adding a try-with-resources API to attach/detach a context without cancelling, so that the two try statements in the example will look more symmetrical. But the syntactic sugar it provides is very minor.

Internal use cases that kick off work asynchronously and use ListenableFuture will use an internal helper utility. We can not easily open source that code because grpc-context must have minimal dependencies and can not use guava.

[zpencer]

CC: @adriancole

[codefromthecrypt]

Tldr I like this because errorprone can warn on lack of close. Wrt how to handle exceptions, the example is honest. Asides: I know @pauldraper attempted to layer capabilities to handle exceptions neatly (in a different lib), cleverly with multiple "throws arity" https://github.com/lucidsoftware/java-thread-context/tree/master/thread-context/src/main/java/com/github/threadcontext/function cc also @nicmunroe Ack grpc context isnt java 8 but the approach could be layered with non java 8 types here or elsewhere. I feel we are getting closer to something ideal, but try/resources idiom is limited. One idea is a compiler or codegen which "fixes" the catch vs autoclose problem. Ex possibly similar to autovalue memoize or retrolambda. In this way we could eliminate the boilerplate by hiding it somehow. Cc @orfjackal @raphw @eamonnmcmanus

[zhangkun83]

LGTM

[zhangkun83]

Maybe also in cancel()'s javadoc refer back to close().

[ejona86]

Get rid of the extra newlines. Take a look at the generated documentation to see how it looks.

[ejona86]

Concerning catching the exception: I think I may encourage people not to (and just cancel(null)/close()).

Basically there's two different operations at play here: cancellation (cancel with error) and cleanup (cancel with null). My hypothesis is that generally when using try-with-resources (aka, you're using blocking-style APIs), you have no need to cancel, even if an exception was thrown. The only time "you're cancelling" is when the Deadline is reached, which provides an exception.

Now, that's not at all true when doing async work. But if you're doing async work you're much less likely to use try-with-resources.

[codefromthecrypt]

One note is that at least with a trace context (usually not using this form but as an example) we often use try with because you can end up with an exception creating or starting a deferred task (ex latter could be a rejection cause). Not sure if such a pattern applies elsewhere or if there is a risk in not knowing the cause.

…

On 25 Oct 2017 8:29 am, "Eric Anderson" ***@***.***> wrote: Concerning catching the exception: I think I may encourage people not to (and just cancel(null)/close()). Basically there's two different operations at play here: cancellation (cancel with error) and cleanup (cancel with null). My hypothesis is that generally when using try-with-resources (aka, you're using blocking-style APIs), you have no need to cancel, even if an exception was thrown. The only time "you're cancelling" is when the Deadline is reached, which provides an exception. Now, that's not at all true when doing async work. But if you're doing async work you're much less likely to use try-with-resources. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3607 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD619K1QVp2B_dgyMTZoIXc4ez6KeQDks5svoDTgaJpZM4QDqyZ> .