Found worker thread-leak

[stamhankar999]

The formatter assumes that it is executed as a CLI, but it actually has a resource leak. I was trying to use it in a service, and the resource leak became apparent with more use. The issue is that Main.formatFiles creates an ExecutorService, but it doesn't shut it down. In addition it would be nice if a user could pass in their own ExecutorService whose lifecycle the user manages. That way, for every invocation of the formatter in a service context, a new ExecutorService need not be created/shutdown.

[cushon]

Main.formatFiles is only intended to be used by the CLI. If you want to use the formatter as a library and invoke it programmatically, I recommend com.google.googlejavaformat.java.

[stamhankar999]

Yes, that's certainly possible, but then I'd have to duplicate much of the surrounding code (e.g. Main.formatFiles, FormatFileCallable) which does exactly what I want it to (other than having the resource leak).

[cushon]

There's some related discussion in #145, this code really isn't intended to be used as a library, in part because it makes decisions about e.g. threading and error handling that clients using the formatter programatically should probably make independently.

[stamhankar999]

There are a few reasons why it should be designed to be used as a library with a CLI wrapper:

• IDE plugins - they're not going to invoke a CLI; they'd want to load the formatting library classes and invoke the formatter on one or more files or file streams that way.
• Maven / gradle plugins - same idea as IDE plugins
• Code formatting servers - this is admittedly an edge case, but that's how I discovered the thread leak. My case was that I had a tree that was badly formatted and I wanted to use git filter-branch to reformat the tree historically and re-write the git repo. That way, commit history is preserved. Using the CLI, it took over 12 hours before I killed the process. After writing a caching formatting server, I was able to rewrite my repo in about 2 hours.

I get that you don't want to expose too much of the internals to others so that you're free to refine the implementation as needed, but as a user, I wouldn't need much: a class like FormatFileCallable, except public and takes one file/input-stream arg and formatting options, and returns a Future (or CompletableFuture) with the formatting result. Or maybe also a variant where the task writes the result file to disk for you.

[plumpy]

it should be designed to be used as a library with a CLI wrapper

To be clear, it is designed exactly that way, and Main is the CLI wrapper you speak of. I wrote the IntelliJ plugin, and we use it as a library just fine, we just use the com.google.googlejavaformat.java classes, as @cushon suggested above.

[plumpy]

Maybe you could remove the public modifier from Main to make it more clear?

[stamhankar999]

@plumpy @cushon I didn't really delve into the Formatter class before; I was focusing on Main.formatFiles, FormatFileCallable, and the non-public things it does, some of which I thought I needed. But knowing that IntelliJ's plugin's entry-point is the Formatter class, I'm now looking there and finding public methods that should fit the bill.

Thanks.