Formatter is now AutoCloseable, so that facilitates FormatterFunc.Closeable works.#284
Formatter is now AutoCloseable, so that facilitates FormatterFunc.Closeable works.#284
FormatterFunc.Closeable works.#284Conversation
jbduncan
left a comment
jbduncan
left a comment
There was a problem hiding this comment.
Hi @nedtwigg. I'm convinced by the reasoning you made in your message above for making these changes, so on that front, this PR LGTM!
My only reservation is with how FormatterStepImpl.Standard has cleanupFormatterFunc whereas FormatterStepImpl.NeverUpToDate doesn't. I suppose my fear is that we may use FormatterStepImpl.NeverUpToDate in the future expecting it to auto-close FormatterFunc.Closeables as well, but then for it to bite us back. But it's quite likely I'm just being a bit too anxious here - we'd probably remember to implement such a method if needed - which is why I've submitted my approval rather than specifically request a change here.
So regardless of whether you decide to make FormatterStepImpl.NeverUpToDate have a cleanupFormatterFunc method or not, consider this as a "LGTM" from me!
… FormatterFunc.Closeable in the future.
|
I think that's a good point worth addressing, addressed in previous commit. |
|
Cheers @nedtwigg! This all definitely LGTM now. 👍 |
3 participants
For a long time, we've had
FormatterFunc.Closeablein the codebase, but with no usages.spotless/lib/src/main/java/com/diffplug/spotless/FormatterFunc.java
Lines 22 to 43 in f622379
The idea was that a
FormatterFuncwith native integration might be holding native handles that would need to be released. Now that we're about to integrate with V8 thanks to #283, we need to make this dead code come to life. The good news is that the abstraction we laid down without a concrete usecase 2 years ago fits the need in #283 perfectly. Aside: terrible idea that we did this without a use-case, the git blame shows it's my fault :'(The bare minimum that we'll need to do is make
Formatterinto anAutoCloseable, which then will cleanup all of itsFormatterStep/FormatterFunc.Formatteris only used in a few places in our entire codebase (8 including tests), so it's an easy change.The next choice is whether
FormatterStepshould also beAutoCloseable. This is problematic, because we use them willy-nilly all over the place. It's a very nice property to be able to use them willy-nilly.It's important to note that the only time a
FormatterStepever needs to be closed is if it has actually been applied, because until then itsFormatterFunchas not been created. The intention of the library is that no one will callFormatterStep.formatexceptFormatter, so lifting the resource-management burden toFormatteris relatively safe.It might be tempting here to think "aha! We don't need SpotlessCache anymore!", which is the mechanism we use for caching ClassLoaders. However, it's important to note that we do this to share ClassLoaders across spotless runs so that the JIT can warmup (we get huge speed improvements compared to creating a destroying classloader for each run of Spotless). So this doesn't affect
ClassLoaderresources, just other kinds of resources.