spring-projects / spring-boot

Spring Boot
https://spring.io/projects/spring-boot
Apache License 2.0
74.4k stars 40.51k forks source link

launch.script should not get in the way of sigterm propagation #15229

Open Johnlon opened 5 years ago

Johnlon commented 5 years ago

https://github.com/spring-projects/spring-boot/blob/069e7f3881bd95a75cae8ddd7abef143ab116296/spring-boot-project/spring-boot-tools/spring-boot-loader-tools/src/main/resources/org/springframework/boot/loader/tools/launch.script#L265

Because the run() function omits the use of "exec" it leaves a useless (??) shell script process running once the java has invoked.

As it stands when this shell parent process receives a SIGTERM then it doesn't pass it on to Java and the Java process is left lying around. This interferes with process management in some instances.

Could the run function be modified to use the "exec" shell command to dispense with the parent process? (see issue 5273)

If we can't modify the existing run for some then can we have an alternative that does an exec or provide an option to modify the behaviour of run, or even an option to customise the start script?

Thanks

See also https://github.com/spring-projects/spring-boot/issues/5273

wilkinsona commented 5 years ago

As you have seen, we have already considered and rejected the suggestion of using exec in #5273. In the absence of an argument that suggests our reasoning was faulty, I don’t think we’re going to change our minds. Would you like to try and make one?

The launch script can already be customised by configuring the build to use a file of your choice. Please see the documentation for details.

Johnlon commented 5 years ago

The argument in the earlier ticket was concerning changing the behaviour of run() which is why I suggested the possibility of having an alternative action, let's call it mode=exec which uses exec under the cover. This approach does not modify existing behaviour, which was the objection raised previously.

--

Whilst I could override the script entirely with my own version I then have the added complexity of doing this across all my builds including the added effort of managing the same script across those builds.

Better just to add a new feature to the script IMHO so everyone can benefit at low cost.

wilkinsona commented 5 years ago

The objection in #5273 was also that it wasn't apparent why it was necessary:

Why do you need to use sh spring-boot-app-exec.jar run as the command? You could make the jar itself executable and use spring-boot-app-exec.jar as run is the default action.

With no information to the contrary, I assume you are in the same situation so the same question applies.

Better just to add a new feature to the script IMHO so everyone can benefit at low cost.

I don't agree that the cost is low. Every configuration option that we add is something that we have to document and that users have to understand when deciding what functionality meets their needs. For a rarely seen problem such as this where a solution already exists (using a custom script that meets your needs or executing the jar differently), it's very difficult to justify adding another another way to do the same thing.

Johnlon commented 5 years ago

I am using the chmod'd jar directly.

I am not doing... sh spring-boot-app-exec.jar

The problem is that the embedded script itself is an invoked shell that hangs around and prevents signal propagation.

The default embedded script clearly doesn't play nicely with signals. I feel that it would be very easy to document this additional feature.

wilkinsona commented 5 years ago

I am using the chmod'd jar directly.

Thanks. When referencing #5273 it would have saved quite a bit of time if you'd pointed out that, while your situation was similar, there was a key difference in what you're doing.

I feel that it would be very easy to document this additional feature.

Writing the documentation may or may not be hard, but that isn't the problem. The problem is the extra complexity that the configuration option introduces. We'd have to describe when you may want the exec variant of run and when you may want the current behaviour, the effect that using exec has in a non-interactive shell, etc.

As I said before, it's difficult to justify adding and documenting that complexity for a problem that, as far as we know, has not affected many people and has other solutions. In addition to being able to use a custom script to add the use of exec, you also have the option of killing the child java process or killing the relevant group of processes with kill -- -<parentPid>, where parentPid is the pid of the script's shell.

I'll flag this one for team attention to see what the other members of the team think.

Johnlon commented 5 years ago

The process group approach only works if there is a separate process group and if the parent process the is able to interoperate with process groups.

For instance if the parent process is java then Process.destroyForcibly() or Process.destroy() do not understand process groups and so only kill the bash wrapper leaving the java process orphaned.

wilkinsona commented 5 years ago

It sounds like there's more to your situation than you've described. This is the first time that you've mentioned that, apparently, you're using Java to shell out and execute another process via the launch script.

To avoid wasting any more time, can you please describe in detail exactly what you're trying to do and the problem that you're facing and ignoring any possible solutions for now.

Johnlon commented 5 years ago

Honestly, I find this incredible. Why should it matter that we have a use case where the process scheduler is a JVM? The limitation still stands and impacts anyone who isn't able to use process groups - for whatever reason.

philwebb commented 5 years ago

Honestly, I find this incredible. Why should it matter that we have a use case where the process scheduler is a JVM? The limitation still stands and impacts anyone who isn't able to use process groups - for whatever reason.

@Johnlon Having more context about the issue really helps us find the best long-term solution that we're going to be comfortable supporting. I agree with @wilkinsona that we don't want to add an additional configuration option just for this one use-case. We already offer the ability to customize the embedded launch script, so it is already possible to embed an alternative that uses exec (if that's what you really want).

I wonder if the use of exec is just one possible solution to the problem. Perhaps using traps might be another option that would allow us propagate the signal to process without needing to use exec.

I'd like to investigate that a little more, but it would certainly help if we have more details about your specific scenario so that we don't implement a fix that's not going to help you.

Johnlon commented 5 years ago

Given that adding exec to the existing function has been ruled out then the next simplest approach is a separate function that does exec. Anything that attempted to properly handle signals with trap would be hard and incomplete. Best approach as originally requested is to fix the signal propagation by eliminating the useless shell process.

philwebb commented 5 years ago

Given that adding exec to the existing function has been ruled out then the next simplest approach is a separate function that does exec.

Can you describe what you mean by this? How will using exec in a separate function prevent the issues discussed in https://github.com/spring-projects/spring-boot/issues/15229#issuecomment-190132375 ?

Anything that attempted to properly handle signals with trap would be hard and incomplete.

I've not looked into it yet so I've no idea how hard or incomplete it will be. Have you tried to do something similar in the past without success?

Best approach as originally requested is to fix the signal propagation by eliminating the useless shell process.

I'm afraid I still don't agree that that's necessarily the best approach for all users.

I think this issue could be more productive if you describe in detail what you are trying to do and what's not working for you. In an earlier comment you said:

For instance if the parent process is java then Process.destroyForcibly() or Process.destroy() do not understand process groups and so only kill the bash wrapper leaving the java process orphaned

That sounds like you're trying to launch an executable JAR from another java process then stop it at a later point. Is that the case? If so, can you share a snippet of code that shows us exactly what you are doing? That will enable us to focus on the actual problem and try to find the least invasive solution that hopefully works for everyone.

Johnlon commented 5 years ago
Process p = Runtime.getRuntime().exec("springbootexec.jar");
p.destroyForcibly();

This will not kill the java subprocess because the springboot shell script gets in the way of the kill signal.

Kill cannot be caught therefore no trap in the shell script can solve this problem.

The only solution is for the intermediate spring boot shell script to not exist and instead for the java process to receive the kill signal directly, this is only possible if the script uses exec.

renatomefi commented 4 years ago

I think majority of people might simply be using java -jar .. directly, as running the lunch script now is kind of useless in a containerized environment, not propagating the signals is a big, very big issue in this case. exec is the best option for cloud native applications, you don't want a pesudo-init system as this script being your pid 1. More here: https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#entrypoint

It could indeed be optional for now, but I'd say the future doesn't look like the current approach!

edit: as in this comment https://github.com/spring-projects/spring-boot/issues/5273#issuecomment-190132375, it might just be outdated, also, by the lack of traction in those issues here I think people just don't bother using launch scripts, would be interesting to know how big is the crowd of people being affect by this vs the ones that just move on without digging the fact we have a design issue going on!

wilkinsona commented 4 years ago

Useless is a rather strong way of putting it, but we do not expect many, if any, people to use the launch script when deploying to a containerized environment. Please note that the problem with signal propagation is, as we understand it, specific to an environment where a separate Java process is orchestrating applications that are launched as fully-executable jars. The vast majority of people using the launch script will be unaffected by this problem, hence it being a low-priority issue for us. For those for whom it is a problem, a custom launch script that meets their needs can be used.

Johnlon commented 4 years ago

It's a big. Should be fixed.

jacko9et commented 1 year ago
Process p = Runtime.getRuntime().exec("springbootexec.jar");
p.destroyForcibly();

This will not kill the java subprocess because the springboot shell script gets in the way of the kill signal.

Kill cannot be caught therefore no trap in the shell script can solve this problem.

The only solution is for the intermediate spring boot shell script to not exist and instead for the java process to receive the kill signal directly, this is only possible if the script uses exec.

I think this issue should at least be documented, after all this is calling a Java program using the Java API and getting unexpected results.