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

Duplicate jvm.options entries generated from a single -Dliberty.jvm.xyz=123 system property in v3.10 #1778

Closed
scottkurz opened this issue Dec 8, 2023 · 3 comments · Fixed by #1779
Assignees

Comments

@scottkurz
Copy link
Member

Running mvn liberty:run -Dliberty.jvm.debug="-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=7777" (to attach the debugger to the server) I got an error and noticed the reason was duplicate entries in the generated jvm.options:

$ cat .../jvm.options
# Generated by liberty-maven-plugin
-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=7777
-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=7777

The problem also happens with dev mode.

LIKELY CAUSE

I'm pretty sure the change here is responsible.

In debugging into StartDebugMojoSupport.loadLibertyConfigFromProperties()

    private void loadLibertyConfigFromProperties() {
        loadLibertyConfigFromProperties(project.getProperties());
        loadLibertyConfigFromProperties(System.getProperties());
    }

I can see the problem.

In the good, normal case (e.g. with v3.9) only the second load here, from the System properties, results in the addition of a new entry.

                    case JVM:        jvmMavenProps.add(value);

In the bad case, I get an entry from the 1st load from project.getProperties() and then a 2nd from the System properties.
(Because jvm.options are a list, not a key-value map, this doesn't get merged into one entry like the other config types).

If I just run liberty:create there's no problem. If I execute 'run' or 'dev' though, we do a second copyConfigFiles AFTER having executed the new (in v3.10) setContainerEngine() call.

SOLUTION

The answer might require doing something more like a clone of these properties? Seems like we have a test gap too.

@scottkurz scottkurz changed the title Duplicate jvm.options entries generated from a single -Dliberty.jvm.xyz=123 system property Duplicate jvm.options entries generated from a single -Dliberty.jvm.xyz=123 system property in v3.10 Dec 8, 2023
@cherylking
Copy link
Member

So I understand in this case we shouldn't add two entries for one property, but in most cases I don't think it would cause a failure. This debug setting is probably somewhat special.

For other options, I believe the last one in the generated file will take precedence (and I think we depend on that behavior for when a particular jvm option is specified both in the pom.xml and from the command line). We even have a test case for it showing that both options appear in the generated file, but the one we expect to take precedence appears last.

@scottkurz
Copy link
Member Author

Interesting..yeah from https://stackoverflow.com/questions/2740725/duplicated-java-runtime-options-what-is-the-order-of-preference it does look like the last one wins... and we can see this with -Dliberty.jvm.print="XX:+PrintFlagsFinal".

I guess a case could be made that we could (should?) eliminate exact duplicates. This way, for an arg that couldn't be duplicated, like the jdwp debug thing, you can have it as each of a Maven property & system property. But, you'd still have a similar issue if you, say, used the two properties with different ports in each (so they wouldn't be identical), and that doesn't seem like it'd be that helpful to matter.

@cherylking cherylking self-assigned this Dec 8, 2023
@cherylking
Copy link
Member

The problem stemmed from code that modified the Properties that were returned from project.getProperties(). That was not the intent of the code. However, I will also change the existing code that adds jvm option entries to the List so that duplicate entries are not added.

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