> On March 5, 2014, 11:39 a.m., Mike Drob wrote: > > core/src/main/java/org/apache/accumulo/core/util/shell/Shell.java, line 460 > > <https://reviews.apache.org/r/18773/diff/1/?file=510868#file510868line460> > > > > Should move the assignment inside of the try-block. > > Bill Havanki wrote: > Why? Even now it's the default no-arg constructor, it can't even throw > anything. (This is my usual idiom, so if there is a standard reason why it's > best to do the assignment in the try that I don't know, I definitely want to > learn it.) > > Sean Busbey wrote: > the default Shell constructor throws IOException. In this case though, it > looks like the try block is just there to handle shutdown. If the constructor > throws there's nothing to shutdown.
Don't know how I missed that constructor. Please disregard my last comment. It is possible for that thread to start before the JLine ConsoleReader constructor throws IOException [1]. If it does, we've got no reference to the instance to tell it to stop the thread, so we're screwed. Ergo, there is no point in having the constructor occur in the try-catch. [1] https://github.com/jline/jline2/blob/master/src/main/java/jline/console/ConsoleReader.java - Bill ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18773/#review36234 ----------------------------------------------------------- On March 5, 2014, 11:29 a.m., Bill Havanki wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18773/ > ----------------------------------------------------------- > > (Updated March 5, 2014, 11:29 a.m.) > > > Review request for accumulo. > > > Bugs: ACCUMULO-2429 > https://issues.apache.org/jira/browse/ACCUMULO-2429 > > > Repository: accumulo > > > Description > ------- > > The JLine 2 ConsoleReader used by Shell spawns a thread which should be > cleaned up when done with the Shell. Otherwise, the thread leaks, taking up > resources when the shell is used programmatically. This commit adds a > shutdown() method to Shell for cleaning up the thread. This enables > ShellServerIT to pass reliably and not flood the OS with leaked threads. > > > Diffs > ----- > > core/src/main/java/org/apache/accumulo/core/util/shell/Shell.java > 850816c7e1673d89ae8fd29818cf3322c3cd8b3b > core/src/test/java/org/apache/accumulo/core/util/shell/ShellConfigTest.java > df4d817aad50e3c34e6de02f68b4345af00bfac6 > > core/src/test/java/org/apache/accumulo/core/util/shell/ShellSetInstanceTest.java > 5a6cc8a0052b03cfa510b80b072408139428ccff > core/src/test/java/org/apache/accumulo/core/util/shell/ShellTest.java > bf203f77d4e6809bacdb82217e3c8d8aaeecbe48 > test/src/test/java/org/apache/accumulo/test/ShellServerIT.java > da094e85ed3ebaeef2ac2cac5beb7b52299a49dc > > Diff: https://reviews.apache.org/r/18773/diff/ > > > Testing > ------- > > Ran shell-related unit tests successfully. Ran ShellServerIT successfully > under both Mac OS X and CentOS 6. Ran shell against live instance to ensure > it starts and stops normally. > > > Thanks, > > Bill Havanki > >
