Issue Details (XML | Word | Printable)

Key: DERBY-214
Type: Bug Bug
Status: Closed Closed
Resolution: Fixed
Priority: Major Major
Assignee: David Van Couvering
Reporter: Kathey Marsden
Votes: 0
Watchers: 0
Operations

If you were logged in you would be able to see more operations.
Derby

Remove System.exit() calls from the DB2jServerImpl.java

Created: 11/Apr/05 10:47 AM   Updated: 26/Jan/06 02:02 AM
Return to search
Component/s: Network Server
Affects Version/s: 10.1.1.0
Fix Version/s: 10.1.1.0

Time Tracking:
Not Specified

File Attachments:
  Size
File Licensed for inclusion in ASF works DERBY-214.diff 2005-05-17 07:02 AM David Van Couvering 6 kB

Resolution Date: 21/May/05 07:14 AM


 Description  « Hide
The System.exit() calls needs to be removed from the
DB2jServerImpl.java as this results in the entire application
(example - Eclipse which is running the Network Server inside
it) getting shut down.



 All   Comments   Work Log   Change History   Subversion Commits      Sort Order: Ascending order - Click to sort in descending order
David Van Couvering added a comment - 28/Apr/05 02:48 AM
Hi, Kathey. I think I can work on this. Is it OK for NetworkServerControl to call System.exit, or does that need to throw an exception as well?

The technique I usually do for a utility that I want callable by another class as well as executable from the command line is for the main() routine to delegate to another public method which throws exceptions and doesn't call System.exit(). Then the main() routine catches any exceptions and calls System.exit(1) if there is an exception or System.exit(0) if there isn't one. Then classes call the other public method and main() is only used from the command-line.

Does that work here?

Thanks,

David

Craig Russell added a comment - 30/Apr/05 09:17 AM
Hi,

I agree that main() should only be used from the command line, and is the only place in the code where System.exit() should be called. The main() method could simply delegate to another method that takes the String args[] parameter. For the purposes of this message, I'll just call it "execute".

All of the utility programs should use the same method name for the "execute" method so it's easy to remember how to invoke it. The "execute" then does the parsing of the args. There are lots of Apache commons arg parsers that could be used in the "execute", but that's just a bit off topic.

Execute should throw exceptions, not ever call System.exit. Since the execute method has the same parameters as main, it's easy to remember how to invoke it.

The second issue is whether the execute method should be static or not. For consistency, I think that non-static would be the way to go. If we offer different methods of executing, then it would be nice to have all of them behave the same in terms of static vs. non-static.

The third issue is where the processing of environment variables should be done. It might be best if the constructor of an instance of the utility class processed the environment variables. That way, regardless of whether main() or a program called the constructor the behavior would be the same.

The fourth issue is whether for ease of use, aother convenience methods could be used. One that takes a single String is desirable. This method, also called execute, could simply parse the input String into a String[] and call the other execute method. An alternative method would take a Map of parameter names and values with the obvious semantics.

I see a few alternatives

1. ij.execute(new String[] {"{"-p", "1567", "-h", "localhost" });
2. ij.setPort(1567); ij.setHost("localhost");ij.execute();
3. ij.execute("-p 1567 -h localhost");
4. Map map = new HashMap(); map.put("-p", "1567"); map.put("-h", "localhost"); ij.execute(Map map);

I can see value in all of these techniques depending on what you have on the caller side of the interface.

Regards,

Craig

David Van Couvering added a comment - 14/May/05 07:01 AM
There are a lot of interesting discussions about how to call classes with main(). However, NetworkServerControl already has a well-defined set of routines that are already a documented public interface. I think that in the case of this particular situation we shouldn't change NetworkServerControl to have a new interface based on any of the proposed models.

There has also been a discussion about moving argument processing out of DB2jServerImpl (now renamed NetworkServerImpl) and into NetworkServerControl. But I think that is beyond the scope of this particular JIRA item.

What I am going to do for this is simply move the System.exit() from NetworkServerImpl to NetworkServerControl, and then I will open two separate items: one to relocate argument processing to NetworkServerControl, and another to adjust the other tools that don't already have a documented public interface to follow a calling model we all agree on.

David

David Van Couvering added a comment - 17/May/05 07:02 AM
Attached is the patch that should resolve this item. I relocated the execute method
from NetworkServerImpl (previously called DB2jServerImpl) to NetworkServerControl.

I modified derbynet/testconnection.java so that it checks and makes sure the exit
status matches what is expected (1 for error, 0 for no error).

Ran derbyall, all tests pass except for tools/dblook_test.java, which was also failing
in my DERBY-243 tree. This is an unrelated diff, I'll be logging a bug for this.

Output of svn status:

M java/drda/org/apache/derby/impl/drda/NetworkServerControlImpl.java
M java/drda/org/apache/derby/drda/NetworkServerControl.java
M java/testing/org/apache/derbyTesting/functionTests/tests/derbynet/testcon
nection.java

Kathey Marsden added a comment - 19/May/05 01:08 PM

I committed this patch.
Sending java\drda\org\apache\derby\drda\NetworkServerControl.java
Sending java\drda\org\apache\derby\impl\drda\NetworkServerControlImpl.java
Sending java\testing\org\apache\derbyTesting\functionTests\tests\derbynet\testconnection.java
Transmitting file data ...
Committed revision 170862.

David Van Couvering added a comment - 21/May/05 07:14 AM
This appears to be fixed,although there was a resulting test failure on Solaris 10 x86 that I am working on (DERBY-303).