[jira] Created: (EXEC-34) Race condition prevent watchdog working using ExecuteStreamHandler

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[jira] Created: (EXEC-34) Race condition prevent watchdog working using ExecuteStreamHandler

JIRA jira@apache.org
Race condition prevent watchdog working using ExecuteStreamHandler
------------------------------------------------------------------

                 Key: EXEC-34
                 URL: https://issues.apache.org/jira/browse/EXEC-34
             Project: Commons Exec
          Issue Type: Bug
         Environment: Windows Vista 64bit, dual core CPU
            Reporter: Marco Ferrante
            Priority: Critical


Consider this test case (in _DefaultExecutorTest_ class):

{noformat}
    /**
     * Start a async process using a stream handler and terminate it manually
     * before the watchdog timeout occurs
     */
    public void testExecuteAsyncWithStreamHandlerAndUserTermination() throws Exception {
        CommandLine cl = new CommandLine(foreverTestScript);
        ExecuteWatchdog watchdog = new ExecuteWatchdog(Integer.MAX_VALUE);
        PumpStreamHandler streamHanlder = new PumpStreamHandler(System.out, System.err);
        exec.setStreamHandler(streamHanlder);
        MockExecuteResultHandler handler = new MockExecuteResultHandler();
        exec.execute(cl, handler);
        // DON'T wait for script to run
        //Thread.sleep(2000);
        // teminate it
        watchdog.destroyProcess();
        assertTrue("Watchdog should have killed the process",watchdog.killedProcess());
    }
{noformat}

It fails (at least in my environment) because when _watchdog.destroyProcess()_ is invoked the external process is not bound to the watchdog yet.

Although there are possible several workarounds, but all of them seem to me very intrusive in the code. So, I prefer some discussion before preparing and submitting a patch.
IMHO, the watchdog should handle a reference to the thread running the process, not to the process itself. In this way, interrupting signals can be transport using default _interrupt()_ method of class _Thread_.


--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (EXEC-34) Race condition prevent watchdog working using ExecuteStreamHandler

JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/EXEC-34?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12662929#action_12662929 ]

Siegfried Goeschl commented on EXEC-34:
---------------------------------------

To be honest I was aware of the issue therefore the "Thread.sleep" statements in the regression tests. Having said that I think this is an artificial use case since I have to wait for some time for any external process to start and finish before I consider the process "hanging".

> Race condition prevent watchdog working using ExecuteStreamHandler
> ------------------------------------------------------------------
>
>                 Key: EXEC-34
>                 URL: https://issues.apache.org/jira/browse/EXEC-34
>             Project: Commons Exec
>          Issue Type: Bug
>         Environment: Windows Vista 64bit, dual core CPU
>            Reporter: Marco Ferrante
>            Assignee: Siegfried Goeschl
>            Priority: Critical
>
> Consider this test case (in _DefaultExecutorTest_ class):
> {noformat}
>     /**
>      * Start a async process using a stream handler and terminate it manually
>      * before the watchdog timeout occurs
>      */
>     public void testExecuteAsyncWithStreamHandlerAndUserTermination() throws Exception {
>         CommandLine cl = new CommandLine(foreverTestScript);
>         ExecuteWatchdog watchdog = new ExecuteWatchdog(Integer.MAX_VALUE);
>         PumpStreamHandler streamHanlder = new PumpStreamHandler(System.out, System.err);
>         exec.setStreamHandler(streamHanlder);
>         MockExecuteResultHandler handler = new MockExecuteResultHandler();
>         exec.execute(cl, handler);
>         // DON'T wait for script to run
>         //Thread.sleep(2000);
>         // teminate it
>         watchdog.destroyProcess();
>         assertTrue("Watchdog should have killed the process",watchdog.killedProcess());
>     }
> {noformat}
> It fails (at least in my environment) because when _watchdog.destroyProcess()_ is invoked the external process is not bound to the watchdog yet.
> Although there are possible several workarounds, but all of them seem to me very intrusive in the code. So, I prefer some discussion before preparing and submitting a patch.
> IMHO, the watchdog should handle a reference to the thread running the process, not to the process itself. In this way, interrupting signals can be transport using default _interrupt()_ method of class _Thread_.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (EXEC-34) Race condition prevent watchdog working using ExecuteStreamHandler

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/EXEC-34?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12769177#action_12769177 ]

Konrad Windszus commented on EXEC-34:
-------------------------------------

The same applies to e.g. isWatching(). I first tried to wait for an asynchronous process to finish, with the help of ExecuteWatchdog.isWatching(), apparently with no success, due to this bug. You should at least document this flaw.

> Race condition prevent watchdog working using ExecuteStreamHandler
> ------------------------------------------------------------------
>
>                 Key: EXEC-34
>                 URL: https://issues.apache.org/jira/browse/EXEC-34
>             Project: Commons Exec
>          Issue Type: Bug
>         Environment: Windows Vista 64bit, dual core CPU
>            Reporter: Marco Ferrante
>            Assignee: Siegfried Goeschl
>            Priority: Critical
>
> Consider this test case (in _DefaultExecutorTest_ class):
> {noformat}
>     /**
>      * Start a async process using a stream handler and terminate it manually
>      * before the watchdog timeout occurs
>      */
>     public void testExecuteAsyncWithStreamHandlerAndUserTermination() throws Exception {
>         CommandLine cl = new CommandLine(foreverTestScript);
>         ExecuteWatchdog watchdog = new ExecuteWatchdog(Integer.MAX_VALUE);
>         PumpStreamHandler streamHanlder = new PumpStreamHandler(System.out, System.err);
>         exec.setStreamHandler(streamHanlder);
>         MockExecuteResultHandler handler = new MockExecuteResultHandler();
>         exec.execute(cl, handler);
>         // DON'T wait for script to run
>         //Thread.sleep(2000);
>         // teminate it
>         watchdog.destroyProcess();
>         assertTrue("Watchdog should have killed the process",watchdog.killedProcess());
>     }
> {noformat}
> It fails (at least in my environment) because when _watchdog.destroyProcess()_ is invoked the external process is not bound to the watchdog yet.
> Although there are possible several workarounds, but all of them seem to me very intrusive in the code. So, I prefer some discussion before preparing and submitting a patch.
> IMHO, the watchdog should handle a reference to the thread running the process, not to the process itself. In this way, interrupting signals can be transport using default _interrupt()_ method of class _Thread_.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Updated: (EXEC-34) Race condition prevent watchdog working using ExecuteStreamHandler

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

     [ https://issues.apache.org/jira/browse/EXEC-34?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Siegfried Goeschl updated EXEC-34:
----------------------------------

    Priority: Minor  (was: Critical)

Changing priority to minor since this is an artificial problem not happening in production - nobody would start a subprocess and immediately kill it.

> Race condition prevent watchdog working using ExecuteStreamHandler
> ------------------------------------------------------------------
>
>                 Key: EXEC-34
>                 URL: https://issues.apache.org/jira/browse/EXEC-34
>             Project: Commons Exec
>          Issue Type: Bug
>         Environment: Windows Vista 64bit, dual core CPU
>            Reporter: Marco Ferrante
>            Assignee: Siegfried Goeschl
>            Priority: Minor
>
> Consider this test case (in _DefaultExecutorTest_ class):
> {noformat}
>     /**
>      * Start a async process using a stream handler and terminate it manually
>      * before the watchdog timeout occurs
>      */
>     public void testExecuteAsyncWithStreamHandlerAndUserTermination() throws Exception {
>         CommandLine cl = new CommandLine(foreverTestScript);
>         ExecuteWatchdog watchdog = new ExecuteWatchdog(Integer.MAX_VALUE);
>         PumpStreamHandler streamHanlder = new PumpStreamHandler(System.out, System.err);
>         exec.setStreamHandler(streamHanlder);
>         MockExecuteResultHandler handler = new MockExecuteResultHandler();
>         exec.execute(cl, handler);
>         // DON'T wait for script to run
>         //Thread.sleep(2000);
>         // teminate it
>         watchdog.destroyProcess();
>         assertTrue("Watchdog should have killed the process",watchdog.killedProcess());
>     }
> {noformat}
> It fails (at least in my environment) because when _watchdog.destroyProcess()_ is invoked the external process is not bound to the watchdog yet.
> Although there are possible several workarounds, but all of them seem to me very intrusive in the code. So, I prefer some discussion before preparing and submitting a patch.
> IMHO, the watchdog should handle a reference to the thread running the process, not to the process itself. In this way, interrupting signals can be transport using default _interrupt()_ method of class _Thread_.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (EXEC-34) Race condition prevent watchdog working using ExecuteStreamHandler

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/EXEC-34?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12898631#action_12898631 ]

Siegfried Goeschl commented on EXEC-34:
---------------------------------------

Well, actually the sample is buggy since the following line is missing

exec.setWatchdog(watchdog)

which injects the newly created 'Process' instance into the watchdog and without *Process' instance 'watchdog.destroy()' has no effect

> Race condition prevent watchdog working using ExecuteStreamHandler
> ------------------------------------------------------------------
>
>                 Key: EXEC-34
>                 URL: https://issues.apache.org/jira/browse/EXEC-34
>             Project: Commons Exec
>          Issue Type: Bug
>         Environment: Windows Vista 64bit, dual core CPU
>            Reporter: Marco Ferrante
>            Assignee: Siegfried Goeschl
>            Priority: Minor
>
> Consider this test case (in _DefaultExecutorTest_ class):
> {noformat}
>     /**
>      * Start a async process using a stream handler and terminate it manually
>      * before the watchdog timeout occurs
>      */
>     public void testExecuteAsyncWithStreamHandlerAndUserTermination() throws Exception {
>         CommandLine cl = new CommandLine(foreverTestScript);
>         ExecuteWatchdog watchdog = new ExecuteWatchdog(Integer.MAX_VALUE);
>         PumpStreamHandler streamHanlder = new PumpStreamHandler(System.out, System.err);
>         exec.setStreamHandler(streamHanlder);
>         MockExecuteResultHandler handler = new MockExecuteResultHandler();
>         exec.execute(cl, handler);
>         // DON'T wait for script to run
>         //Thread.sleep(2000);
>         // teminate it
>         watchdog.destroyProcess();
>         assertTrue("Watchdog should have killed the process",watchdog.killedProcess());
>     }
> {noformat}
> It fails (at least in my environment) because when _watchdog.destroyProcess()_ is invoked the external process is not bound to the watchdog yet.
> Although there are possible several workarounds, but all of them seem to me very intrusive in the code. So, I prefer some discussion before preparing and submitting a patch.
> IMHO, the watchdog should handle a reference to the thread running the process, not to the process itself. In this way, interrupting signals can be transport using default _interrupt()_ method of class _Thread_.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.