Inconsistent signal-handling behavior in java_stub_template / java_binary

[JoshRosen]

Based on manual code reading, I believe that the shell wrapper scripts generated from java_stub_template.txt have a signal-handling bug: it looks like the behavior of passing of signals from the bash wrapper to java is dependent on the string length of the child JVM's classpath and I fear that this may cause subtle, hard-to-diagnose bugs if a classpath length increase can trigger such a significant behavior change.

To spot the problem, let's take a look at java_stub_template.txt as of the current master commit:

At the very bottom of the file, we have this branch:

if is_windows && (("${#CLASSPATH}" > ${CLASSPATH_LIMIT} )); then
  create_and_run_classpath_jar "local_jdk/bin/jar.exe"
elif (("${#CLASSPATH}" > ${CLASSPATH_LIMIT})); then
  create_and_run_classpath_jar "local_jdk/bin/jar"
else
  exec $JAVABIN -classpath $CLASSPATH "${ARGS[@]}"
fi

In create_and_run_classpath_jar, Java is invoked as

  # Execute JAVA command
  $JAVABIN -classpath "$MANIFEST_JAR_FILE" "${ARGS[@]}"
  exit_code=$?
  rm -f "$MANIFEST_FILE"
  rm -f "$MANIFEST_JAR_FILE"
  exit $exit_code

This file does not use bash's trap built-in, so in this case signals like SIGTERM will not be propagated to the child java process.

If a SIGTERM is sent to the bash wrapper script then it will be propagated to java in the common case where exec is used, but if the classpath becomes sufficiently long then the create_and_run_classpath_jar path will be taken and signals will not be propagated.

To fix this, I think that Bazel needs to make proper use of trap or signal handlers in create_and_run_classpath_jar to ensure that signals are consistently forwarded to the child process. We can't simply use exec because we need to perform the rm -rf calls to clean up the temporary manifest files. It turns out that this can be subtle and hard to get right: I found a good blog post at https://veithen.github.io/2014/11/16/sigterm-propagation.html which illustrates some of the pitfalls involved. Also see https://unix.stackexchange.com/questions/146756/forward-sigterm-to-child-in-bash/444676#444676, linked from a comment on that blog post.

It appears that the Linux/Darwin version of this bug was introduced in 102ce6d (which fixed #3069) because the Linux/Darwin "large classpath" case was using exec prior to that patch's changes.

I spotted this problem through manual code reading rather than through an actual failure / bug reproduction: I encountered a similar signal-handling issue in one of my my own Bazel rules (which generates a similar, but much simpler, wrapper script) and decided to look at Bazel's scripts to see whether they used exec to address this signal-handling problem.

[ulfjack]

This is really odd. I have just seen the same problem in test-setup.sh. However, I did not have to follow the referenced blog post, but just adding a trap handler for SIGTERM seems to make it work correctly. Anyone working on this might want to try that first (I'm cc'ing this bug in the patch, so it should show up when it's submitted).

[github-actions]

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 2+ years. It will be closed in the next 14 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-bazeler". Please reach out to the triage team (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

[github-actions]

This issue has been automatically closed due to inactivity. If you're still interested in pursuing this, please reach out to the triage team (@bazelbuild/triage). Thanks!