-
Notifications
You must be signed in to change notification settings - Fork 852
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix 846 Formatter leaks threads and memory #847
Conversation
5fcb5e0
to
7e12876
Compare
7e12876
to
db89e6a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix!
core/src/test/java/com/google/googlejavaformat/java/MainTest.java
Outdated
Show resolved
Hide resolved
3671975
to
e61f7d9
Compare
e61f7d9
to
0ca1e9b
Compare
@cushon do I need to do anything to get the |
Nope, thanks! I can take it from here |
|
||
// #846 Clean up any resources and threads created by the executorService. | ||
final boolean completedShutdown = | ||
MoreExecutors.shutdownAndAwaitTermination(executorService, 100, TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking to use a slightly longer deadline here, maybe 5 seconds.
This is just an FYI--I can make the change, no need to upload another snapshot.
// before shutdown, so no tasks should be left over by the time we shut down the executor | ||
// service. | ||
throw new IllegalStateException( | ||
"Failed to complete all formatting tasks and shut down the formatter."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should print the message to stderr and return with a non-zero exit code, similar to the error handling on other code paths here.
I've signed the CLA.