Comment 1 Martin Oberhuber 2007-02-23 11:56:51 EST

I don't think that's major.

First of all, dstore users would use dstore processes normally, and with ssh this works fine - I tested it. Second, there is a simple workaround (connect something else first).

Comment 2 David McKnight 2007-07-23 17:07:26 EDT

The problem is that HostShellProcessAdapter.exitValue() gets called it the shell process service init method after running a check to check something.  This calls IHostShell.exitValue() to determine whether a shell is done:

	public synchronized int exitValue() {
	  if (hostShell.isActive())
		throw new IllegalThreadStateException();
	
		// No way to tell what the exit value was.
		// TODO it would be possible to get the exit value
		// when the remote process is started like this:
		//   sh -c 'remotecmd ; echo "-->RSETAG<-- $?\"'
		// Then the output steram could be examined for -->RSETAG<-- to get the exit value.
		return 0;
	}

The problem is that a shell may not yet have finished but the exception gets thrown anyway.  If we wait a little longer, then the shell will complete.   A simple solution to this would be to insert a sleep after the first isActive() and check again although that's not a perfect solution.  Any other ideas?   

Comment 3 Martin Oberhuber 2007-07-24 09:31:40 EDT

(In reply to comment #2)
The exitValue() method must work like that (i.e. throw an exception) in order to be compatible with java.lang.Process.

In the concrete case, we do not get an exit value from the remote anyways. I see no sense waiting for it (at least not in the current Thread) and thus blocking execution of other commands - since even if we'd get it, it would just be used for printing an error message.

Thus I'd vote for modifying the calling code like this (note that the HostSHellProcessAdapter p must be final for this):


Job waiter = new Job("LinuxShellProcessWaiter") {
   public void run() {
      try {
         p.waitFor();
         if (p.exitValue()!=0) {
            /*etc as right now*/
         }
      } catch(InterruptedException e) { 
         return Status.CANCEL_STATUS;
      }
      return Status.OK_STATUS;
   }
};
waiter.schedule();

Testing this, you should then verify that the remote shells actually do exit eventually -- if not it's a bug in dstore.

Comment 4 David McKnight 2007-07-24 11:26:35 EDT

I've taken Martin's suggestion in using the job to wait for the exit value.

Comment 5 Martin Oberhuber 2007-07-24 11:42:20 EDT

Dave I think there are two more occurrences of p.exitValue() in the shell processes plugin to be fixed the same way. Also, please check that the Job we create is not visible to the user. You might have to do

   waiter.setSystem(true);

Before you do that, please verify that the Jobs eventually go away. We don't want to have memory leaking because the shells never terminate.

Reopening till all this is clarified.

Comment 6 David McKnight 2007-07-24 12:23:51 EDT

(In reply to comment #5)
> Dave I think there are two more occurrences of p.exitValue() in the shell
> processes plugin to be fixed the same way. Also, please check that the Job we
> create is not visible to the user. You might have to do
> 
>    waiter.setSystem(true);

The only issue seemed to be the startup but your right that we should deal with this similarly in other uses of p.exitValue(). I've committed the fixes for this.

> 
> Before you do that, please verify that the Jobs eventually go away. We don't
> want to have memory leaking because the shells never terminate.
> 

The jobs do go away.