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

Image doesn't place nice with Java Debug Wire Protocol #160

Closed
jnehlmeier opened this issue Sep 4, 2023 · 5 comments · Fixed by #162
Closed

Image doesn't place nice with Java Debug Wire Protocol #160

jnehlmeier opened this issue Sep 4, 2023 · 5 comments · Fixed by #162
Assignees

Comments

@jnehlmeier
Copy link

When starting jetty image (in my case 12.0.0) using

JAVA_OPTIONS="-agentlib:jdwp=transport=dt_socket,server=y,address=*:33333,suspend=n"

the generate-jetty-start.sh script executes DRY_RUN=$(/docker-entrypoint.sh "$@" --dry-run) with $DRY_RUN now containing a java message as the first line:

Listening for transport dt_socket at address: 33333
/opt/java/openjdk/bin/java -Djava.io.tmpdir=/tmp/jetty ......

The generate-jetty-start.sh now prepends exec to that variable content and executes a regex on it. The regex removes the first line completely (which does contain the exec <java message>) and only keeps the second line.

The result is that Java now does not run as PID 1 inside the container and docker stop now kills the container after 30 seconds as no signals reach java/jetty. Also for local development you can not use CTRL + C to terminate the container anymore.

@jnehlmeier
Copy link
Author

Possible solution:

Instead of

DRY_RUN=$(/docker-entrypoint.sh "$@" --dry-run)
echo "exec $DRY_RUN" \
    | egrep '[^ ]*java .*org\.eclipse\.jetty\.xml\.XmlConfiguration ' \
    | sed -e 's/ -Djava.io.tmpdir=[^ ]*//g' -e 's/\\$//' \
    > $JETTY_START

the script could do

DRY_RUN=$(/docker-entrypoint.sh "$@" --dry-run)
# keeps the java command line and removes possible JVM output messages
DRY_RUN=$(echo "$DRY_RUN" | egrep '[^ ]*java .*org\.eclipse\.jetty\.xml\.XmlConfiguration ' | sed -e 's/ -Djava.io.tmpdir=[^ ]*//g' -e 's/\\$//')
# ensures to exec the final command
echo "exec $DRY_RUN" > $JETTY_START

@lachlan-roberts lachlan-roberts self-assigned this Sep 5, 2023
@jnehlmeier
Copy link
Author

I am wondering what would have happened if I used suspend=y. I guess the container would never end up in state started because the generation of jetty.start would never complete, right? The JVM executing the --dry-run command would wait until a JDWP connection has been established to that JVM. In that case my proposed solution above would not work.

Maybe using JAVA_OPTIONS for the JVM executing terminal commands like --dry-run or --list-config isn't a good idea. Personally I would only want JAVA_OPTIONS to take effect on the JVM that actually runs jetty server to handle requests.

@lachlan-roberts
Copy link
Contributor

@jnehlmeier I have added a test case for this JAVA_OPTIONS which tests the PID of the java process. PR #162 contains your suggest change fixes the test case.

Maybe using JAVA_OPTIONS for the JVM executing terminal commands like --dry-run or --list-config isn't a good idea. Personally I would only want JAVA_OPTIONS to take effect on the JVM that actually runs jetty server to handle requests.

Yeah the idea of that has been floated around before, I might try and separate out the JAVA_OPTIONS for the --dry-run and the ones used to run the server.

lachlan-roberts added a commit that referenced this issue Sep 6, 2023
Issue #160 - add fix for generate-jetty-start.sh file
@jnehlmeier
Copy link
Author

@lachlan-roberts Nice to have a test now. Because the container did launch it wasn't obvious what caused the issue.

Yeah the idea of that has been floated around before, I might try and separate out the JAVA_OPTIONS for the --dry-run and the ones used to run the server.

If you go that route it would be great if the JAVA_OPTIONS used to run the server would be adjustable dynamically without rebuilding the image. Basically executing java $JAVA_OPTIONS -jar start.jar .... to start the server. This could be done by applying sed on the output of --dry-run to insert the $JAVA_OPTIONS variable. I mentioned something like that already in #151 (comment) because of JPMS.

@lachlan-roberts
Copy link
Contributor

I'll open a new issue for this. But we will need to review any potential impacts of not letting the --dry-run know about the JAVA_OPTIONS if we are to go with that approach.

Opened #164.

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 a pull request may close this issue.

2 participants