Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Process Listener API #413

Closed
wants to merge 1 commit into from
Closed

Process Listener API #413

wants to merge 1 commit into from

Conversation

Speiger
Copy link

@Speiger Speiger commented Oct 20, 2024

Creates a Listener API for Process instances done by this library. That way giving library users more control over how processes are stopped.

The Stopper API itself never guarantees that the Processes are stopped and this assures that the guarantee can be self implemented.

Edit: This should solve #383

@Speiger Speiger changed the title Master Process Listener API Oct 20, 2024
@kokorin
Copy link
Owner

kokorin commented Oct 23, 2024

@Speiger thank you for submitting this PR. Could you clarify why custom implementation of Stopper is not enough to expose Process?
I see actually that there are 2 problems:

  1. ffmpeg may continue running when JVM exits, this can be solved by Phantom or Weak references
  2. Stopper may leave ffmpeg running even if force_stop() is used. AFAIK there is specific API was added in Java 9

This link may be useful: https://fw-geekycoder.blogspot.com/2014/01/how-to-force-kill-process-in-java.html

@Speiger
Copy link
Author

Speiger commented Oct 23, 2024

@kokorin the reason why it is not enough is:
You get 0 notification when a process is done unless you implement a external tracker that does it automatically for you and even then you would have to figure out which one process it is in your own tracker impl. So a custom stopper is needed.
On top of that FFprobe doesn't have a stopper impl.

Also i realized i shouldn't have used github code spaces to do this step.

ffmpeg may continue running when JVM exits, this can be solved by Phantom or Weak references

Unless you implement a finalize method (which is depricated at this point) that will not work. Since i had already cases in my app where i started 20 ish heavy FFmpeg tasks and killed the JVM task by closing the app (which triggered a System.exit(0) in my impl) and the program kept running even so the jvm task was stopped gracefully.

This link may be useful: https://fw-geekycoder.blogspot.com/2014/01/how-to-force-kill-process-in-java.html

I am running in Java21 with my application, and from what I have seen is: ForceDestroy simply calls destroy and does nothing else in the implementation. There are also no indicators that there is a different implementation present either.

From what i figured out, Process:destroy is working just fine.
Also there are other use cases for giving access to processes, for example for tracking the tasks you are issuing.

@kokorin
Copy link
Owner

kokorin commented Oct 23, 2024

You get 0 notification when a process is done unless you implement a external tracker that does it automatically for you and even then you would have to figure out which one process it is in your own tracker impl. So a custom stopper is needed.

Could you clarify a bit how it should work? I mean how that external tracker actually tracks and kills sub-process?

Unless you implement a finalize method (which is depricated at this point) that will not work.

It's also possible to use JVM shutdown hook for that.

From what i figured out, Process:destroy is working just fine.

Originally it was the approach for Stopper. But it didn't work all the times, I don't already remember details, sorry.
I think it it's possible to use 2 approaches together: first to send qq to ffmpeg's input and then invoke Process.destroy().

Also there are other use cases for giving access to processes, for example for tracking the tasks you are issuing.

Could you clarify how having Process instance can help in tracking tasks? With the same success one could use Ffmpeg or FfmpegFuture for that purpose.

@Speiger
Copy link
Author

Speiger commented Oct 23, 2024

Could you clarify a bit how it should work? I mean how that external tracker actually tracks and kills sub-process?

My implementation is simply a ConcurrentSet for example, that keeps track of all tasks,
and when the App is shutdown then it will simply iterate over these and call: process.destroy and after that System.exit.
That takes care of ensuring that all processes are killed.

Tracking for example means ram usage/CPU usage of a process using the OSHI library.
Stuff thats just interesting for devs.

It's also possible to use JVM shutdown hook for that.

Still a hook where the devs could decide which processes have to be waited for and which not would be nice :)
But your solution is what we ask for but "You have to manage it" instead of the user which we suggest because it would reduce the load of work you have to do to ensure minimum features that make it viable.

I think it it's possible to use 2 approaches together: first to send qq to ffmpeg's input and then invoke Process.destroy().

That's also a way, but a more demanding on you which why we suggested giving access for those who want to manage it themselves and give you less work :)

Could you clarify how having Process instance can help in tracking tasks? With the same success one could use Ffmpeg or FfmpegFuture for that purpose.

It would allow us to track more than "Oh its running or its now finished" with the Oshi library we can also track its memory usage, cpu usage, writing/reading etc.
So giving access to the process would allow people to do fancy stuff too.

Again this is just a accessibility suggestion that reduces the work you have to do to the bare minimum so people can do stuff if they want to and you can still maintain it easily :)

@kokorin
Copy link
Owner

kokorin commented Oct 27, 2024

@Speiger sorry for late reply, you convinced me. Please, pay attention that:

  1. the PR should pass all checks before it can be merged into.
  2. Pr should be based on develop branch not master and should target also develop branch.
  3. Last but not least is testing, consider adding a test that would verify that ProcessListener is actually invoked

@Speiger
Copy link
Author

Speiger commented Oct 27, 2024

@kokorin yeah that's totally fine.
I am just really busy myself atm i will adjust things next week when i have a bit more free time and less of a timeline biting me in the neck.

@Speiger Speiger mentioned this pull request Nov 2, 2024
@Speiger
Copy link
Author

Speiger commented Nov 2, 2024

Closing because #416

@Speiger Speiger closed this Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants