On Sun, 27 Apr 2025 20:56:59 GMT, Kevin Walls <kev...@openjdk.org> wrote:
>> test/jdk/sun/jvmstat/monitor/MonitoredVm/MonitorVmStartTerminate.java line >> 78: >> >>> 76: private static final int PROCESS_COUNT = 10; >>> 77: private static final int ARGS_ATTEMPTS = 5; >>> 78: private static final int REMOVE_TIMEOUT_SECONDS = 300; >> >> I'm still not convinced we need a large timeout here. The default timeout is >> 120 seconds, so jtreg will time out the test before waitForRemoval() times >> out itself. > > Yes -- Maybe just take out this extra timeout. I'm being too respectful of > things that have been in the test forever, but aren't of much use. Some > tests have their own timeouts, when jtreg can just time them out. > > Checking again, the timeouts I've seen from this tests are from the jtreg > harness, not the loop that had a 1000 second timeout, or even when I change > it, it's still jtreg giving the timeout. Should remove it. > > ARGS_ATTEMPTS could be even longer. If it completely fails to read a process > arguments, we are at risk of failing later due to timeout. We may as well > try harder, this is the part that might fail and need retrying when the > processes are slow to start. Yes, I think the issue is that hasMainArgs() does not succeed after ARGS_ATTEMPTS (3), which probably means 30 seconds given the 10 second attach timeout. Once this happens, waitForRemoval() will timeout no matter how long you wait. Setting ARGS_ATTEMPTS to 5 might be enough to alleviate this problem, although you might want to go with something more like 10 just to make sure we can eliminate this as the cause if it comes up again. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24843#discussion_r2062869665
