Adding @PreDestroy annotation to public facing shutdown methods

[dekelpilli]

Issue #, if available:
N/A

Description of changes:
Pretty simple change, but not necessarily desired. This won't impact simple (unframeworked) codebases like the example, but could be a nice addition to people using frameworks to manage object states.

I did not add the annotation to SQSMessageConsumer as it already implements AutoCloseable, and its close method already calls shutdowon(). Furthermore, the SQSMessageConsumer's shutdown() method is already called by AmazonSQSRequesterClient's shutdown().

A potential downsides is reducing user control if they are using such a framework and want to shutdown either client in a non-graceful way, but I think that is very fringe and outweighed by the benefit.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[robin-aws]

This is a good idea! Initially I thought you were introducing a dependency on something like Spring, but I was surprised to learn this annotation is part of the JRE itself. It's actually too bad this can't easily be added to the built-in SDK clients as well.

I think this should be on SQSMessageConsumer as well, actually. I am assuming that the annotation will only trigger automatic shutdown if the object itself is injected by a framework such as Spring. Even then, shutdown() should be idempotent so duplicate invocations are safe. I definitely wouldn't worry about intentionally shutting down uncleanly.

Is it idiomatic to add this to the interface method, or the implementing classes, or both?

[dekelpilli]

Yeah, it's just one of those annotations that's intended to be used by frameworks (like @Inject and @PostConstruct).

Is it idiomatic to add this to the interface method, or the implementing classes, or both?

I wasn't able to find any documentation on a standard around this, I believe it's probably just down to the implementation. For example, in Spring:

@Bean
public AmazonSQSRequester amazonSQSRequester() {
 /* some code that returns an instance of AmazonSQSRequesterClient */
}

So does Spring read the return type defined in the method and do a getAnnotaitons(), or does it do getClass().getMethods().getAnnotations()? As there's no standard that I could find, I figured it's best to be safe and do it on both.

Anyway, sure, I'll add it to the consumer. What are you thoughts on @PostContstruct on the consumer's start() method? The reason I didn't is because I can definitely imagine scenarios where the consumption would happen based on some trigger/schedule.

[robin-aws]

@PostConstruct@ on start() is very appealing too, but I don't think changing that would be backwards-compatible even assuming start() can safely be invoked multiple times. If client code is failing to invoke shutdown() and we invoke it automatically when a framework is shutdown, I think that's a reasonable and safe improvement. I'm a lot more concerned about clients that are explicitly calling start() later on and finding the earlier invocation causes regressions or errors, though.

I'd love to add it whenever there is a major version bump though. Perhaps cut a separate issue for tracking?

[dekelpilli]

Sure, added - #22