On Fri, 12 Jul 2024 09:22:54 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
> Can I please get a review of this test-only change which proposes to fix the > test timeout reported in https://bugs.openjdk.org/browse/JDK-8334167? > > The JBS issue has comments which explains what causes the timeout. The commit > in this PR addresses those issues by updating the test specific > `ClassFileTransformer` to only instrument application specific class instead > of all (core) classes. The test was introduced several years back to verify > the feature introduced in https://bugs.openjdk.org/browse/JDK-6263319. As > such, the proposed changes in this PR continue to test that feature - we now > merely use application specific class' native method to verify the semantics > of that feature. > > Additional cleanups have been done in the test to make sure that if any > exception does occur in the ClassFileTransformer then it does get recorded > and that then causes the test to fail. > > With this change, I have run tier1 through tier6 and the test passes. > Additionally, without this change I've run the test with a test repeat of 100 > with virtual threads enabled and the test hangs occasionally (as expected). > With this proposed fix, I have then run the test with virtual threads, around > 300 times and it hasn't failed or hung in any of those instances. I think the changes looks okay. It narrows to instrumenting a specific test class, thus removing all the side effects of the transformer code generating wrapper methods for JDK classes. The cleanups looks okay, test/jdk/java/lang/instrument/NativeMethodPrefixApp.java line 44: > 42: * @comment The test uses asmlib/Instrumentor.java which relies on > ClassFile API PreviewFeature. > 43: * @run main/native/timeout=240 NativeMethodPrefixApp roleDriver > 44: * @comment The test uses a higher timeout to prevent test timeouts noted > in JDK-6528548 Is /timeout=240 (and the comment) needed now. If I read the old issue correctly it was due to use a JDK mounted on a network file system. test/jdk/java/lang/instrument/libNativeMethodPrefix.c line 24: > 22: */ > 23: > 24: #include "jni.h" I assume this needs `#include <stdio.h>`. ------------- PR Review: https://git.openjdk.org/jdk/pull/20154#pullrequestreview-2176976546 PR Review Comment: https://git.openjdk.org/jdk/pull/20154#discussion_r1677385666 PR Review Comment: https://git.openjdk.org/jdk/pull/20154#discussion_r1677383597
