On Thu, 29 Jan 2026 10:21:25 GMT, Jaikiran Pai <[email protected]> wrote:
>> Can I please get a review of this test-only change which addresses >> intermittent failures in >> `java/rmi/server/Unreferenced/leaseCheckInterval/LeaseCheckInterval.java`? >> >> The `@summary` in that test's test definition about what this test does: >> >>> @summary When the "java.rmi.dgc.leaseValue" system property is set to a >>> value much lower than its default (10 minutes), then the server-side >>> user-visible detection of DGC lease expiration-- in the form of >>> Unreferenced.unreferenced() invocations and possibly even local garbage >>> collection (including weak reference notification, finalization, etc.)-- >>> may be delayed longer than expected. While this is not a spec violation >>> (because there are no timeliness guarantees for any of these garbage >>> collection-related events), the user might expect that an unreferenced() >>> invocation for an object whose last client has terminated abnormally >>> should occur on relatively the same time order as the lease value >>> granted. >> >> In its current form, the test uses a lease expiry of 10 seconds, launches a >> trivial `java` application which looks up the bound object from the registry >> and then terminates itself. After launching that trivial java application, >> the test then waits for 20 seconds, expecting that the >> `Unreferenced.unreferenced()` callback (upon lease expiry of 10 seconds) >> will be called within those 20 seconds. This wait intermittently fails >> because the `Unreferenced.unreferenced()` doesn't get called within those 20 >> seconds. >> >> Experiments show that the reason for these intermittent failures is due to >> the `SelfTerminator` application which does the registry lookup (and which >> involves connection establishment and communication over a socket) can >> sometimes take several seconds (5 or more for example). That effectively >> means that by the time this `SelfTerminator` starts its termination after >> the lookup, it's already several seconds into the "wait()" in the test. >> >> The commit in this PR cleans up the test to more accurately track the >> duration of how long it took between the lease expiry and the >> `Unreferenced.unreferenced()` callback to be invoked. Additionally, just to >> make the test more robust, the maximum expected duration has been increased >> to 60 seconds instead of 20 seconds. Given the text in the test's summary, I >> think this increase is still within the expectations of how long it takes >> for the callback to be invoked after the client has exited abnormally. >> >> The test continue... > > Jaikiran Pai has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains nine additional > commits since the last revision: > > - Stuart's review - no need to write start time in a file > - merge latest from master branch > - merge latest from master branch > - merge latest from master branch > - Mark's suggestion - move the duration check to separate method > - merge latest from master branch > - Mark's review - use CountDownLatch > - merge latest from master branch > - 8170896: TEST_BUG: > java/rmi/server/Unreferenced/leaseCheckInterval/LeaseCheckInterval.java > failed with unreferenced() not invoked after 20.0 seconds Thank you Stuart for the review. > The observation is that the timeout logic is entirely within the parent JVM. > The child's role is simply to obtain a lease and halt immediately. The parent > JVM could be changed to call JavaVM.waitFor() to await the child's > termination and to obtain a timestamp. This would remove the need to create a > temporary file into which the child writes a timestamp from which the parent > reads the timestamp. In addition, waitFor() allows the parent to check the > child's exit status and signal an error if the status is nonzero. > > I guess one could argue that having the child obtain the timestamp > immediately after the registry call is closer to the beginning of the actual > lease. However, I don't think this makes much of a practical difference. What you suggest is reasonable and simpler as well. I've updated the PR to remove the part where we used to write the start time to a file and then read it. With the updated log messages in this PR, I think if this continues to fail intermittently, we will have some useful log messages which can then help us decide if we have to track the start time more closely in the SelfTerminator process. > Another small cleanup item: is it necessary to have a @modules declaration in > the test? I don't see anything in the test that uses RMI internals. (The Test > Library might need this stuff, though, not sure.) I gave that a try, but turns out the call to a test library class in this test: > TestLibrary.getRegistryPort(localRegistry); requires the use of these explicit "--add-exports" (through the `@modules`) because `TestLibrary` references those internal classes. ------------- PR Comment: https://git.openjdk.org/jdk/pull/28919#issuecomment-3816774725
