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

Incompatible with jacoco due to shutdown race condition #7344

Closed
XakepSDK opened this issue Dec 29, 2021 · 8 comments · Fixed by #7374
Closed

Incompatible with jacoco due to shutdown race condition #7344

XakepSDK opened this issue Dec 29, 2021 · 8 comments · Fixed by #7374
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@XakepSDK
Copy link

Jetty version(s)
jetty-maven-plugin 11.0.7

Java version/vendor (use: java -version)
openjdk version "17.0.1" 2021-10-19
OpenJDK Runtime Environment (build 17.0.1+12)
OpenJDK 64-Bit Server VM (build 17.0.1+12, mixed mode)

OS type/version
Arch Linux

Description
I'm using jetty and jacoco to generate integration tests coverage.
Jetty shutdowns in post-integration-test phase and jacoco's report-integration goal runs at verify phase.
When i run stop goal in post-integration-test phase, plugin quits immediately even if jetty's VM is not shut down yet.
Because VM is not shutdown yet, jacoco agent that is attached to jetty's VM has not finished writing coverage data file.
While jetty's VM is not shutdown yet, jacoco in project that started and stopped jetty starts collecting report.
It may lead to 3 results:

  1. VM shutdown before jacoco executed: good coverage results
  2. VM not shutdown before jacoco executed: may lead to EOFException in Jacoco or empty coverage report, as if no coverage was done

Jetty maven plugin should check if VM is shut down or not. stopWait tag does not help, feels like it doesn't check anything.

How to reproduce?

  1. Project properties:
<jacoco.output.dir>${project.build.directory}/jacoco/output</jacoco.output.dir>
<jacoco.ut.execution.data.file>${jacoco.output.dir}/jacoco-unit-tests.exec</jacoco.ut.execution.data.file>
<jacoco.it.execution.data.file>${jacoco.output.dir}/jacoco-integration-tests.exec</jacoco.it.execution.data.file>
<jetty.maven.plugin.version>11.0.7</jetty.maven.plugin.version>
  1. Jacoco plugin config:
<plugin>
    <groupId>org.jacoco</groupId>
    <artifactId>jacoco-maven-plugin</artifactId>
    <version>${jacoco.maven.plugin.version}</version>
    <executions>
        <execution>
            <id>pre-unit-test-execution</id>
            <goals>
                <goal>prepare-agent</goal>
            </goals>
            <configuration>
                <destFile>${jacoco.ut.execution.data.file}</destFile>
                <propertyName>surefire.jacoco.args</propertyName>
            </configuration>
        </execution>
        <execution>
            <id>post-unit-test-execution</id>
            <phase>test</phase>
            <goals>
                <goal>report</goal>
            </goals>
            <configuration>
                <dataFile>${jacoco.ut.execution.data.file}</dataFile>
                <outputDirectory>${project.reporting.outputDirectory}/jacoco-unit-test-coverage-report</outputDirectory>
            </configuration>
        </execution>
        <execution>
            <id>pre-integration-test-execution</id>
            <phase>pre-integration-test</phase>
            <goals>
                <goal>prepare-agent-integration</goal>
            </goals>
            <configuration>
                <destFile>${jacoco.it.execution.data.file}</destFile>
                <propertyName>failsafe.jacoco.args</propertyName>
            </configuration>
        </execution>
        <execution>
            <id>post-integration-test-execution</id>
            <phase>verify</phase>
            <goals>
                <goal>report-integration</goal>
            </goals>
            <configuration>
                <dataFile>${jacoco.it.execution.data.file}</dataFile>
                <outputDirectory>${project.reporting.outputDirectory}/jacoco-integration-test-coverage-report</outputDirectory>
            </configuration>
        </execution>
    </executions>
</plugin>
  1. Jetty plugin config:
<plugin>
    <groupId>org.eclipse.jetty</groupId>
    <artifactId>jetty-maven-plugin</artifactId>
    <version>${jetty.maven.plugin.version}</version>
    <configuration>
        <httpConnector>
            <port>8080</port>
        </httpConnector>
        <stopKey>quit</stopKey>
        <stopPort>9000</stopPort>
        <stopWait>90</stopWait>
    </configuration>
    <executions>
        <execution>
            <id>start-jetty</id>
            <phase>pre-integration-test</phase>
            <goals>
                <goal>start</goal>
            </goals>
            <configuration>
                <deployMode>FORK</deployMode>
                <jvmArgs>${failsafe.jacoco.args}</jvmArgs>
            </configuration>
        </execution>
        <execution>
            <id>stop-jetty</id>
            <phase>post-integration-test</phase>
            <goals>
                <goal>stop</goal>
            </goals>
        </execution>
    </executions>
</plugin>

Workaround:
Put some plugin in-between that might halt execution for a split second, like this.
If just having this plugin in-between does not help, launch maven with -Dwait=true
It will ask you to press enter, wait a sec or two and press enter.

<plugin>
    <groupId>net.stickycode.plugins</groupId>
    <artifactId>wait-maven-plugin</artifactId>
    <version>1.5</version>
    <executions>
        <execution>
            <phase>post-integration-test</phase>
            <goals>
                <goal>wait</goal>
            </goals>
            <configuration>
                <promptMessage>Just chilling?</promptMessage>
            </configuration>
        </execution>
    </executions>
</plugin>
@XakepSDK XakepSDK added the Bug For general bugs on Jetty side label Dec 29, 2021
@janbartel janbartel self-assigned this Jan 4, 2022
@janbartel
Copy link
Contributor

@XakepSDK did you try setting the stopWait parameter on the stop goal? That is the time in seconds that the stop plugin will wait to see a confirmation that jetty has stopped. https://www.eclipse.org/jetty/documentation/jetty-11/programming-guide/index.html#jetty-stop-goal

@janbartel
Copy link
Contributor

@XakepSDK I'm going to close this issue, as it seems to me that stopWait is exactly the solution to the problem you reported. If this is not the case, please reopen the issue and provide more information that would help diagnose.

@XakepSDK
Copy link
Author

XakepSDK commented Jan 11, 2022

Hi again! I'm back, i had no internet, lockdowns =(
Yes, i tried stopWait.
Jacoco VM agent writes to it's file as long as VM is alive. When i use stopWait, jacoco client side starts reading file before it's fully written. If stopWait will wait for VM shutdown, then jacoco will read file after it was fully written.

@janbartel
Copy link
Contributor

@XakepSDK oh sorry, I didn't see the stopWait in your config. I see. Well, the problem is that the start plugin code that forks jetty - and thus can know the processid of the child - has already terminated by the time the stop plugin runs. I guess it's possible for the forker to write out the processid to a file that the stop plugin could read, however the management of unrelated processes is riddled with race conditions.

@XakepSDK
Copy link
Author

XakepSDK commented Jan 11, 2022

Can this ticket be reopened?
I can think of this solution:

  1. Stop socket should write current PID in stdout
  2. Stop plugin should read PID and send stop signal
  3. Stop plugin should wait until given PID stops existing in process list. Listing processes should be possible with new java, not sure about Java 8

Or we could read stdout of PID and wait until it's EOF, but i guess that would bring too much platform specific code.

@janbartel
Copy link
Contributor

@XakepSDK I'm trying it with the file first, as that doesn't require any changes to jetty code outside of the jetty-maven-plugin. Generating the pid would require changes in the ShutdownMonitor class outside of the jetty-maven-plugin. It's doable, but changing what is generated might mess up other folks who are parsing the output of the ShutdownMonitor and not expecting to deal with the new message. I'm reopening.

@janbartel janbartel reopened this Jan 11, 2022
janbartel added a commit that referenced this issue Jan 12, 2022
Signed-off-by: Jan Bartel <janb@webtide.com>
janbartel added a commit that referenced this issue Jan 13, 2022
Signed-off-by: Jan Bartel <janb@webtide.com>
janbartel added a commit that referenced this issue Jan 15, 2022
Signed-off-by: Jan Bartel <janb@webtide.com>
janbartel added a commit that referenced this issue Jan 18, 2022
Signed-off-by: Jan Bartel <janb@webtide.com>
janbartel added a commit that referenced this issue Feb 10, 2022
janbartel added a commit that referenced this issue Feb 21, 2022
* Issue #7344 Make plugin wait for forked jetty process to stop

Signed-off-by: Jan Bartel <janb@webtide.com>
@janbartel janbartel reopened this Feb 22, 2022
@janbartel
Copy link
Contributor

Need to reopen this issue: when using the JettyStopMojo from in the same process (ie bound stop to a build phase for non-forked jetty execution), cannot call Process.onExit() .

janbartel added a commit that referenced this issue Feb 24, 2022
* Issue #7344 Fix stop goal for non-forked usage
joakime pushed a commit that referenced this issue Feb 25, 2022
* Issue #7344 Make plugin wait for forked jetty process to stop

Signed-off-by: Jan Bartel <janb@webtide.com>
(cherry picked from commit 0b33877)
joakime pushed a commit that referenced this issue Feb 25, 2022
* Issue #7344 Fix stop goal for non-forked usage

(cherry picked from commit 34c21ff)
@janbartel
Copy link
Contributor

Fixed via #7639 amongst others.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants