Geronimo
  1. Geronimo
  2. GERONIMO-4525

No effective exit code for all Windows commands

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1.3
    • Fix Version/s: 2.1.4, 2.2
    • Component/s: commands
    • Security Level: public (Regular issues)
    • Labels:
      None
    • Environment:

      MS Windows

    • Patch Info:
      Patch Available

      Description

      There are multiple problems in the current Windows batch commands (including geronimo.bat, startup.bat, etc.)

      • It's not recommended to define an environment variable with the name ERRORLEVEL. See [1].
      • Set a value to ERRORLEVEL has no effect to the exit code of the batch command (so the documented exit code "0" and "1" are not actually there).
      • The value of the ERRORLEVEL variable will also get unset when the "@endlocal" command is called.

      [1] http://blogs.msdn.com/oldnewthing/archive/2008/09/26/8965755.aspx

        Activity

        Hide
        Jack Cai added a comment -

        This patch uses the real "errorlevel" to check error conditions. It also ensures all commands exit with a meaningful code: 0 means success, and non-zero means failure.

        Show
        Jack Cai added a comment - This patch uses the real "errorlevel" to check error conditions. It also ensures all commands exit with a meaningful code: 0 means success, and non-zero means failure.
        Hide
        Jack Cai added a comment -

        Enforcing process exit code with "cmd /c exit /b n", because "exit /b n" does not do that. Tested and works well.

        Show
        Jack Cai added a comment - Enforcing process exit code with "cmd /c exit /b n", because "exit /b n" does not do that. Tested and works well.
        Hide
        Jarek Gawor added a comment -

        I committed some changes based on your patch to trunk (revision 741689) that I would like you to review before merging to branches/2.1. There were two issues with the patch:

        1) We can't just call seterrorlevel.bat file. That assumes the batch file exists in the current working directory so running any Geronimo script from directory outside Geronimo bin directory would fail (at least in some cases).

        2) Calling seterrorlevel.bat 0 is unnecessary. A call to @setlocal enableextensions will reset it.

        3) A call to cmd /c exit /b %errorlevel% at the end of the batch files also seems unnecessary. If errorlevel is already set why set it again?

        Show
        Jarek Gawor added a comment - I committed some changes based on your patch to trunk (revision 741689) that I would like you to review before merging to branches/2.1. There were two issues with the patch: 1) We can't just call seterrorlevel.bat file. That assumes the batch file exists in the current working directory so running any Geronimo script from directory outside Geronimo bin directory would fail (at least in some cases). 2) Calling seterrorlevel.bat 0 is unnecessary. A call to @setlocal enableextensions will reset it. 3) A call to cmd /c exit /b %errorlevel% at the end of the batch files also seems unnecessary. If errorlevel is already set why set it again?
        Hide
        Jack Cai added a comment -

        Thanks Jarek for your help!

        Ack to 1) & 2).

        3) It is because the cmd executing the batch script exits with the exit code of the last executied command, not the value of "errorlevel". Take geronimo.bat as an example, the last command is "@endlocal", so the exit code will always be "0". Calling "cmd /cd exit /b %errorlevel%" will set the exit code properly. This is helpful when the batch file is called from a program, for example from a Java program (use Runtime.getRuntime().exec()).

        Some other comment on your commit -

        geronimo.bat

        • better add "cmd /cd exit /b 1" at line 253, to indicate user input errors
        • better add "cmd /cd exit /b 0" at line 309, to reset the errorlevel, because some previous calls to "set XXX=" will result in errorlevel 1

        service_pr.bat

        • better add "cmd /cd exit /b 1" at line 167, to indicate user input errors
        • remove unnecessary line 226-234
        Show
        Jack Cai added a comment - Thanks Jarek for your help! Ack to 1) & 2). 3) It is because the cmd executing the batch script exits with the exit code of the last executied command, not the value of "errorlevel". Take geronimo.bat as an example, the last command is "@endlocal", so the exit code will always be "0". Calling "cmd /cd exit /b %errorlevel%" will set the exit code properly. This is helpful when the batch file is called from a program, for example from a Java program (use Runtime.getRuntime().exec()). Some other comment on your commit - geronimo.bat better add "cmd /cd exit /b 1" at line 253, to indicate user input errors better add "cmd /cd exit /b 0" at line 309, to reset the errorlevel, because some previous calls to "set XXX=" will result in errorlevel 1 service_pr.bat better add "cmd /cd exit /b 1" at line 167, to indicate user input errors remove unnecessary line 226-234
        Hide
        Jarek Gawor added a comment -

        Committed all changes to service_pr.bat and committed the change at line 253 to geronimo.bat as you suggested (revision 742810). Can you explain why the change at 309 is necessary? I'm pretty sure that ERRORLEVEL will be reset when %_EXECJAVA% will be executed.

        As to 3) I'll experiment some more to be sure but @endlocal was not resetting the ERRORLEVEL for me. But @setlocal was.

        Show
        Jarek Gawor added a comment - Committed all changes to service_pr.bat and committed the change at line 253 to geronimo.bat as you suggested (revision 742810). Can you explain why the change at 309 is necessary? I'm pretty sure that ERRORLEVEL will be reset when %_EXECJAVA% will be executed. As to 3) I'll experiment some more to be sure but @endlocal was not resetting the ERRORLEVEL for me. But @setlocal was.
        Hide
        Jarek Gawor added a comment - - edited

        Here's a simple batch and Java code that I used to experiment to whether resetting ERRORLEVEL to 0 or calling is cmd /c exit /b %errorlevel%:

        Test.java:

        public class Test {
        
            public static void main(String [] args) throws Exception {
                System.out.println(args[0]);
                System.exit(Integer.parseInt(args[0]));
            }
        }
        

        Test.bat:

        @echo off
        
        @setlocal enableextensions
        
        set foo=
        echo %errorlevel%
        
        java Test %1
        echo %errorlevel%
        
        @endlocal
        echo %errorlevel%
        

        Then when you execute Test.bat 0 or Test.bat 50 and check echo %ERRORLEVEL% after executing it you should see that the %ERRORLEVEL% is set to the right value without doing anything extra. So based on this I think resetting the ERRORLEVEL or calling cmd /c exit /b %errorlevel%: at the end of the batch file is unnecessary.

        Show
        Jarek Gawor added a comment - - edited Here's a simple batch and Java code that I used to experiment to whether resetting ERRORLEVEL to 0 or calling is cmd /c exit /b %errorlevel%: Test.java: public class Test { public static void main( String [] args) throws Exception { System .out.println(args[0]); System .exit( Integer .parseInt(args[0])); } } Test.bat: @echo off @setlocal enableextensions set foo= echo %errorlevel% java Test %1 echo %errorlevel% @endlocal echo %errorlevel% Then when you execute Test.bat 0 or Test.bat 50 and check echo %ERRORLEVEL% after executing it you should see that the %ERRORLEVEL% is set to the right value without doing anything extra. So based on this I think resetting the ERRORLEVEL or calling cmd /c exit /b %errorlevel%: at the end of the batch file is unnecessary.
        Hide
        Jack Cai added a comment - - edited

        Why reset errorlevel at line 309 in geronimo.bat -
        because issuing "geronimo.bat run" will call "start java.exe ...", which will not reset errorlevel.

        Why call "cmd /c exit /b %errorlevel%" at the end of each batch script -
        to set the exit code of the SCRIPT. For example, the below Java code calls the gsh.bat and then check its exit code. Without "cmd /c exit /b %errorlevel%", the gsh.bat will always exit with code 0 (I already explained the reason in the previous comment), and there is NO way to read the errorlevel value because the process executing the script already ends.

        import java.io.BufferedReader;
        import java.io.File;
        import java.io.IOException;
        import java.io.InputStream;
        import java.io.InputStreamReader;
        
        public class ProcessTest {
            static class StreamGobbler extends Thread
            {
                InputStream is;
                String type;
                
                StreamGobbler(InputStream is, String type)
                {
                    this.is = is;
                    this.type = type;
                }
                
                public void run()
                {
                    try
                    {
                        InputStreamReader isr = new InputStreamReader(is);
                        BufferedReader br = new BufferedReader(isr);
                        String line=null;
                        while ( (line = br.readLine()) != null)
                            System.out.println(type + ">" + line);    
                        } catch (IOException e) {
                            e.printStackTrace();  
                          }
                }
            }
        
            /**
             * @param args
             */
            public static void main(String[] args) throws Exception {
                String[] cmds = new String[] {
                        "D:\\Dev\\wasce-server-2.1.1.1\\bin\\gsh.bat",
                        "-c",
                        "geronimo/wait-for-server -u system -w manager -t 10" };
                Process ps = Runtime.getRuntime().exec(cmds, null,
                        new File("D:\\Dev\\wasce-server-2.1.1.1\\bin"));
        
                StreamGobbler errorGobbler = new 
                    StreamGobbler(ps.getErrorStream(), "ERROR");            
                
                StreamGobbler outputGobbler = new 
                    StreamGobbler(ps.getInputStream(), "OUTPUT");
                    
                errorGobbler.start();
                outputGobbler.start();
        
                // Print the exit code
                int exitVal = ps.waitFor();
                System.out.println("ExitValue: " + exitVal);
            }
        
        }
        

        Hope this helps. Thanks again Jarek!

        Show
        Jack Cai added a comment - - edited Why reset errorlevel at line 309 in geronimo.bat - because issuing "geronimo.bat run" will call "start java.exe ...", which will not reset errorlevel. Why call "cmd /c exit /b %errorlevel%" at the end of each batch script - to set the exit code of the SCRIPT. For example, the below Java code calls the gsh.bat and then check its exit code. Without "cmd /c exit /b %errorlevel%", the gsh.bat will always exit with code 0 (I already explained the reason in the previous comment), and there is NO way to read the errorlevel value because the process executing the script already ends. import java.io.BufferedReader; import java.io.File; import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; public class ProcessTest { static class StreamGobbler extends Thread { InputStream is; String type; StreamGobbler(InputStream is, String type) { this .is = is; this .type = type; } public void run() { try { InputStreamReader isr = new InputStreamReader(is); BufferedReader br = new BufferedReader(isr); String line= null ; while ( (line = br.readLine()) != null ) System .out.println(type + ">" + line); } catch (IOException e) { e.printStackTrace(); } } } /** * @param args */ public static void main( String [] args) throws Exception { String [] cmds = new String [] { "D:\\Dev\\wasce-server-2.1.1.1\\bin\\gsh.bat" , "-c" , "geronimo/wait- for -server -u system -w manager -t 10" }; Process ps = Runtime .getRuntime().exec(cmds, null , new File( "D:\\Dev\\wasce-server-2.1.1.1\\bin" )); StreamGobbler errorGobbler = new StreamGobbler(ps.getErrorStream(), "ERROR" ); StreamGobbler outputGobbler = new StreamGobbler(ps.getInputStream(), "OUTPUT" ); errorGobbler.start(); outputGobbler.start(); // Print the exit code int exitVal = ps.waitFor(); System .out.println( "ExitValue: " + exitVal); } } Hope this helps. Thanks again Jarek!
        Hide
        Jarek Gawor added a comment -

        Yes, this was very helpful. It's weird how this works.... Anyway, I'll add those cmd /e exit /b calls in. Thanks for helping me to understand this!

        Show
        Jarek Gawor added a comment - Yes, this was very helpful. It's weird how this works.... Anyway, I'll add those cmd /e exit /b calls in. Thanks for helping me to understand this!
        Hide
        Jarek Gawor added a comment -

        Added the cmd /c exit /b %errorlevel% calls to the end of all scripts and added cmd /c exit 0 to geronimo.bat (revision 742843). Thanks again! I'll work on merging these changes to branches/2.1. Let me know if I missed anything.

        Show
        Jarek Gawor added a comment - Added the cmd /c exit /b %errorlevel% calls to the end of all scripts and added cmd /c exit 0 to geronimo.bat (revision 742843). Thanks again! I'll work on merging these changes to branches/2.1. Let me know if I missed anything.
        Hide
        Jack Cai added a comment -

        Looks good to me now. Thanks Jarek!

        Show
        Jack Cai added a comment - Looks good to me now. Thanks Jarek!
        Hide
        Jarek Gawor added a comment -

        Merged the batch file updates to branches/2.1 (revision 743129). Thanks again!

        Show
        Jarek Gawor added a comment - Merged the batch file updates to branches/2.1 (revision 743129). Thanks again!
        Hide
        Jack Cai added a comment -

        Looks good to me now. Thanks Jarek. Closing it.

        Show
        Jack Cai added a comment - Looks good to me now. Thanks Jarek. Closing it.

          People

          • Assignee:
            Jarek Gawor
            Reporter:
            Jack Cai
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development