timolee / jodconverter

Automatically exported from code.google.com/p/jodconverter
0 stars 0 forks source link

There should be a option/feature to kill open pipes which were not closed while shuting down the application #52

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
There should be a method which allows an application to kill open pipes. 

Mails:

Hi Daniel,

2009/8/2 Daniel Manzke <daniel.manzke@googlemail.com>:
>
> while developing I force some times the problem, that I kill my
> application and forgot to shutdown it correctly. So the office process
> is still running.
> When I then restart my application I can't connect because the process
> is still running.
>
> So I've implemented a method to find the process which was opened and
> kill it. Then I can connect to OOo. For me it's possible to find the
> running process, because I'm still using the same pipe names.
>
> It would be nice, if we could implement some kind of function in the
> jodconverter, so this would be possible with the standard?!
>
You're not the first person to request such a feature actually. Please
open an issue

 http://code.google.com/p/jodconverter/issues/list

I don't think it should be the default but only an option in any case
- think about what would happen otherwise if you accidentally start
two applications using the same OOo pipe name. The second app would
kill the OOo process started by the first app, and chaos would ensue.

Kind regards

Original issue reported on code.google.com by daniel.m...@googlemail.com on 5 Aug 2009 at 9:08

GoogleCodeExporter commented 9 years ago

Original comment by mirko.na...@gmail.com on 12 Apr 2010 at 8:04

GoogleCodeExporter commented 9 years ago
I second this! I want it too. 

Original comment by shervin.asgari@gmail.com on 13 Apr 2010 at 6:54

GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
Here is a working patch for this issue.
The only thing I am unsure about is the PureJavaProcessManager. I had to put a 
if(process != null) because I am setting null in the processManager, because I 
don't know how to retrieve the Process object processManager.kill(null, 
existingPid);

If you have suggestion on improvements, then please let me know

Original comment by shervin.asgari@gmail.com on 10 Mar 2011 at 10:30

Attachments:

GoogleCodeExporter commented 9 years ago
Yep that's precisely why implementing this feature is not trivial: there's no 
pure Java way of terminating an external process.

With your suggested patch PureJavaProcessManager.kill() would do nothing, 
OfficeProcess will try and start a new office process even though another one 
is already running, and the app will be in an usuable state without warning the 
user.

At least it should exit with an error saying that auto-killing stale office 
processes is not supported when using PureJavaProcessManager.

I'm not really satisfied with the current ProcessManager implementations anyway 
- they're not very robust, and it's a pain to have to support many different 
versions of operating systems. But that's a separate issue...

Original comment by mirko.na...@gmail.com on 13 Mar 2011 at 2:53

GoogleCodeExporter commented 9 years ago
Would it be possible to create a new java process in the 
PureJavaProcessManager, so that we could correctly kill that java process?

Do you want me to implement your exception suggestion and resubmit a patch? 

Original comment by shervin.asgari@gmail.com on 13 Mar 2011 at 3:54

GoogleCodeExporter commented 9 years ago
You get a Process instance in Java only if you start a process from the same 
Java app. If the process was left there by a previous run there's no (pure 
Java) way you can get a handle to it.

For a related idea about process management see issue #82.

Original comment by mirko.na...@gmail.com on 13 Mar 2011 at 4:06

GoogleCodeExporter commented 9 years ago
FWIW, this is my workaround:

manager = configuration.buildOfficeManager();
manager.start();
Runtime.getRuntime().addShutdownHook(new Thread("JODConverterShutdownHook") {
  @Override
  public void run() {
    manager.stop();
  }
});

Original comment by bsi....@gmail.com on 14 Mar 2011 at 8:59

GoogleCodeExporter commented 9 years ago
That's not a workaround, that's something users should always do: make sure 
stop() is called when the app terminates.

Original comment by mirko.na...@gmail.com on 14 Mar 2011 at 9:36

GoogleCodeExporter commented 9 years ago
Oh, I seem to have misunderstood the topic of this thread, then.

Original comment by bsi....@gmail.com on 14 Mar 2011 at 9:50

GoogleCodeExporter commented 9 years ago
Right, you are correct. I still have some time for this project this week. 
Anything you want me to do?

Original comment by shervin.asgari@gmail.com on 14 Mar 2011 at 9:51

GoogleCodeExporter commented 9 years ago
I can try to refactor the code to use SIGAR if you want?

Original comment by shervin.asgari@gmail.com on 14 Mar 2011 at 9:52

GoogleCodeExporter commented 9 years ago
Or maybe you haven't, and other users requesting this new option are also 
simply not closing their apps properly, in which case I'd be tempted to say the 
new option is not needed.

Original comment by mirko.na...@gmail.com on 14 Mar 2011 at 9:52

GoogleCodeExporter commented 9 years ago
My last comment was in response to Comment 11.

@Shervin: yep feel free to have a look at Sigar if you like. May be better to 
continue discussions on the group rather than here.

Original comment by mirko.na...@gmail.com on 14 Mar 2011 at 9:55

GoogleCodeExporter commented 9 years ago
I tried adding the shutdown hooks, but still it will not kill the pipes if my 
application server is killed. That's why I still think its a good idea to have 
the feature of autokilling the open pipes. 

Mirko: I guess you haven't really decided if you want the feature in or not, 
but in case of the latter, then you shouldn't have created the issue. In case 
of the former, can I commit the diff in trunk?

Original comment by shervin.asgari@gmail.com on 22 Mar 2011 at 10:41

GoogleCodeExporter commented 9 years ago
In fact I didn't create this issue. :)

Personally I never felt the need for this feature. I've never seen an office 
process still running after stopping my Java app - unless I manually kill -9 
the Java app, but in that case it's pretty obvious that I need to manually kill 
any process started by that app as well. And I don't want to encourage bad 
practices like not closing apps properly.

That said if users keep asking for it then I can reluctantly agree to add it as 
an option. That's my position.

Regarding your patch, I pointed out an issue with PureJavaProcessManager, and 
the need to revise process managers anyway. Maybe you can commit the changes to 
the Sigar branch, and then I'll do the merge?

Original comment by mirko.na...@gmail.com on 22 Mar 2011 at 11:23

GoogleCodeExporter commented 9 years ago
Sounds good. I can commit it in sigar branch.

I was thinking a little about issue 40 - WebServices. I can try to do that, but 
I would like to update the pom to use Java 6, since Java 5 is EOF, and possible 
Java EE 6 jaxrs restful webservices. But I don't know what your take is on that?

Original comment by shervin.asgari@gmail.com on 22 Mar 2011 at 11:29

GoogleCodeExporter commented 9 years ago
Code is not commited in sigar branch

Original comment by shervin.asgari@gmail.com on 22 Mar 2011 at 11:54

GoogleCodeExporter commented 9 years ago
Setting newly-added status "FixedInBranch" to be able to tell what still needs 
to be merged into trunk.

Original comment by mirko.na...@gmail.com on 27 Mar 2011 at 11:13

GoogleCodeExporter commented 9 years ago

Original comment by mirko.na...@gmail.com on 27 Mar 2011 at 1:12