Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Sat, 2 Mar 2024 04:33:11 GMT, Chris Plummer wrote:
>> Two variants:
>>
>> public void runHelper(CommandExecutor executor, String cmd, Path path) {
>>try {
>>run(executor, cmd, path);
>>} catch (Throwable t) {
>> t.printStackTrace(System.out);
>> throw t;
>>} finally {
>>Files.deleteIfExists(path);
>>}
>> }
>>
>> or
>>
>> public void runHelper(CommandExecutor executor, String cmd, Path path) {
>>try {
>>run(executor, cmd, path);
>>} finally {
>>try {
>> Files.deleteIfExists(path);
>>} catch (Throwable t) {
>>
>> t.printStackTrace(System.out);
>>}
>>}
>> }
>
> The one issue with the first variant is that when run() throws an exception,
> it will be printed twice, assuming deleteIfExists() does not throw an
> exception. Basically you either get the exception printed twice, or you get
> the exception printed once plus whatever exception deleteIfExists() throws
> also printed (which is ok).
>
> I think your 2nd example will work, but it's kind of confusing because if you
> got into the finally block via an exception, you then might throw another
> exception, which is handled in the finally block, and then (implicitly) the
> original exception is rethrown as you exit the finally block. However, if
> there was no exception from run() but deleteIfExists() throws an exception,
> the exception is printed, but does not cause the test to fail. I don't think
> we want that.
Hello Chris, I haven't followed this PR and this is just a drive by comment
about the exception handling discussion. In some other areas of our JDK tests,
for situations like this, we do the following:
Throwable failure = null;
try {
run(new JMXExecutor(), "Compiler.perfmap " + path.toString(), path);
} catch (Throwable t) {
failure = t;
} finally {
try {
Files.deleteIfExists(path);
} catch (IOException ioe) {
if (failure != null) {
// add the IOException as a suppressed exception to the original
failure
// and rethrow the original
failure.addSuppressed(ioe);
throw failure;
}
// no failures in the run(), but deleting the file failed, rethrow this
IOException
throw ioe;
}
}
This way both the original test failure (if any) as well as the failure in the
finally block (if any) are propagated and made available in the failure report.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17992#discussion_r1509896340
Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Sat, 2 Mar 2024 01:40:39 GMT, Serguei Spitsyn wrote:
>>> Q2: Can we use Files.deleteIfExists(path) in the finally block of the
>>> run() method instead?
>>
>> I don't think that changes anything. If run() throws an exception and then
>> Files.deleteIfExists() throws an exception from the finally block, the
>> original exception is lost.
>>
>>> One more approach is to wrap run() call into the runHelper() method:
>>
>> Same issue with potentially losing the original exception.
>
> Two variants:
>
> public void runHelper(CommandExecutor executor, String cmd, Path path) {
>try {
>run(executor, cmd, path);
>} catch (Throwable t) {
> t.printStackTrace(System.out);
> throw t;
>} finally {
>Files.deleteIfExists(path);
>}
> }
>
> or
>
> public void runHelper(CommandExecutor executor, String cmd, Path path) {
>try {
>run(executor, cmd, path);
>} finally {
>try {
> Files.deleteIfExists(path);
>} catch (Throwable t) {
>
> t.printStackTrace(System.out);
>}
>}
> }
The one issue with the first variant is that when run() throws an exception, it
will be printed twice, assuming deleteIfExists() does not throw an exception.
Basically you either get the exception printed twice, or you get the exception
printed once plus whatever exception deleteIfExists() throws also printed
(which is ok).
I think your 2nd example will work, but it's kind of confusing because if you
got into the finally block via an exception, you then might throw another
exception, which is handled in the finally block, and then (implicitly) the
original exception is rethrown as you exit the finally block. However, if there
was no exception from run() but deleteIfExists() throws an exception, the
exception is printed, but does not cause the test to fail. I don't think we
want that.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17992#discussion_r1509883828
Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Sat, 2 Mar 2024 00:44:02 GMT, Chris Plummer wrote:
>> test/hotspot/jtreg/serviceability/dcmd/compiler/PerfMapTest.java line 103:
>>
>>> 101: } while(Files.exists(path));
>>> 102: run(new JMXExecutor(), "Compiler.perfmap " + path.toString(),
>>> path);
>>> 103: }
>>
>> Q1: It is not clear why the file is not deleted in this case. Do I miss
>> anything?
>> Q2: Can we use `Files.deleteIfExists(path)` in the `finally block` of the
>> `run()` method instead?
>>
>>
>> public void run(CommandExecutor executor, String cmd, Path path) {
>>try {
>>
>>} finally {
>>Files.deleteIfExists(path);
>>}
>> }
>>
>> One more approach is to wrap `run()` call into the `runHelper()` method:
>>
>> public void runHelper(CommandExecutor executor, String cmd, Path path) {
>>try {
>>run(executor, cmd, path);
>>} finally {
>>Files.deleteIfExists(path);
>>}
>> }
>
>> Q2: Can we use Files.deleteIfExists(path) in the finally block of the run()
>> method instead?
>
> I don't think that changes anything. If run() throws an exception and then
> Files.deleteIfExists() throws an exception from the finally block, the
> original exception is lost.
>
>> One more approach is to wrap run() call into the runHelper() method:
>
> Same issue with potentially losing the original exception.
Two variants:
public void runHelper(CommandExecutor executor, String cmd, Path path) {
try {
run(executor, cmd, path);
} catch (Throwable t) {
t.printStackTrace(System.out);
throw t;
} finally {
Files.deleteIfExists(path);
}
}
or
public void runHelper(CommandExecutor executor, String cmd, Path path) {
try {
run(executor, cmd, path);
} finally {
try {
Files.deleteIfExists(path);
} catch (Throwable t) {
t.printStackTrace(System.out);
}
}
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17992#discussion_r1509803616
Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Sat, 2 Mar 2024 00:14:22 GMT, Serguei Spitsyn wrote: > Q2: Can we use Files.deleteIfExists(path) in the finally block of the run() > method instead? I don't think that changes anything. If run() throws an exception and then Files.deleteIfExists() throws an exception from the finally block, the original exception is lost. > One more approach is to wrap run() call into the runHelper() method: Same issue with potentially losing the original exception. - PR Review Comment: https://git.openjdk.org/jdk/pull/17992#discussion_r1509758594
Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Fri, 23 Feb 2024 21:55:15 GMT, Chris Plummer wrote: > PerfMapTest.java issues the Compiler.perfmap jcmd with a filename argument to > write the perfmap to. It does this in 3 different modes. 2 of the modes > result in a perfmap file being left in the tmp directory that is not removed > after the test executes (and should be removed). The 3rd mode creates the > perfmap file in the directory specified by the test.dir property, and is ok > to leave the file there. I've added code to delete the /tmp files that are > created. > > I did a bit of extra testing by hand. I created `/tmp/perf-.map` as > root. As expected the Files.deleteIfExists() call threw an exception due to > the lack of permissions to delete the file. However, I then realized the file > had a size of 0, which means the test was not even doing a proper job of > testing that the perfrmap jcmd was working. In other words, the test should > have failed long before getting to the Files.deleteIfExists(), so I added a > size check to make sure it fails when the file is not written to. Looks good but posted a question. test/hotspot/jtreg/serviceability/dcmd/compiler/PerfMapTest.java line 103: > 101: } while(Files.exists(path)); > 102: run(new JMXExecutor(), "Compiler.perfmap " + path.toString(), > path); > 103: } Q: It is not clear why the file is not deleted in this case. Do I miss anything? - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17992#pullrequestreview-1912339995 PR Review Comment: https://git.openjdk.org/jdk/pull/17992#discussion_r1509721128
Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Fri, 23 Feb 2024 21:55:15 GMT, Chris Plummer wrote: > PerfMapTest.java issues the Compiler.perfmap jcmd with a filename argument to > write the perfmap to. It does this in 3 different modes. 2 of the modes > result in a perfmap file being left in the tmp directory that is not removed > after the test executes (and should be removed). The 3rd mode creates the > perfmap file in the directory specified by the test.dir property, and is ok > to leave the file there. I've added code to delete the /tmp files that are > created. > > I did a bit of extra testing by hand. I created `/tmp/perf-.map` as > root. As expected the Files.deleteIfExists() call threw an exception due to > the lack of permissions to delete the file. However, I then realized the file > had a size of 0, which means the test was not even doing a proper job of > testing that the perfrmap jcmd was working. In other words, the test should > have failed long before getting to the Files.deleteIfExists(), so I added a > size check to make sure it fails when the file is not written to. I thought about execution of Files.deleteIfExists(path) in the case of test fails, but decided that it is not so important. Also, test always might fail with crash when no finally block is executed. So I am fine with current way. - PR Comment: https://git.openjdk.org/jdk/pull/17992#issuecomment-1974089555
RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is
Please, review this fix correcting the JVMTI `RawMonitorWait()` implementation. The `RawMonitorWait()` is using the the `jt->is_interrupted(true)` to update the interrupt status of the interrupted waiting thread. The issue is that when it calls `jt->is_interrupted(true)` to fetch and clear the `interrupt status` of the virtual thread, this information is not propagated to the `java.lang.Thread` instance. In the `VirtualThread` class implementation the `interrupt status` for virtual thread is stored for both virtual and carrier threads. It is because the implementation of object monitors for virtual threads is based on pinning virtual threads, and so, always operates on carrier thread. The fix is to clear the interrupt status for both virtual and carrier `java.lang.Thread` instances. Testing: - tested with new test `hotspot/jtreg/serviceability/jvmti/vthread/InterruptRawMonitor` which is passed with the fix and failed without it - ran mach5 tiers 1-6 - Commit messages: - fix trailing space in libInterruptRawMonitor.cpp - 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is Changes: https://git.openjdk.org/jdk/pull/18093/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18093&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8325187 Stats: 157 lines in 4 files changed: 155 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18093.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18093/head:pull/18093 PR: https://git.openjdk.org/jdk/pull/18093
Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Fri, 23 Feb 2024 21:55:15 GMT, Chris Plummer wrote:
> PerfMapTest.java issues the Compiler.perfmap jcmd with a filename argument to
> write the perfmap to. It does this in 3 different modes. 2 of the modes
> result in a perfmap file being left in the tmp directory that is not removed
> after the test executes (and should be removed). The 3rd mode creates the
> perfmap file in the directory specified by the test.dir property, and is ok
> to leave the file there. I've added code to delete the /tmp files that are
> created.
>
> I did a bit of extra testing by hand. I created `/tmp/perf-.map` as
> root. As expected the Files.deleteIfExists() call threw an exception due to
> the lack of permissions to delete the file. However, I then realized the file
> had a size of 0, which means the test was not even doing a proper job of
> testing that the perfrmap jcmd was working. In other words, the test should
> have failed long before getting to the Files.deleteIfExists(), so I added a
> size check to make sure it fails when the file is not written to.
The error handling in this test is a real PITA. I just realized that
Files.deleteIfExists() should really be in a finally block. But doing so means
that if it throws an exception, then any exception that was already thrown
during run() is lost. So, for example, let's say you don't have access to the
file and it already has contents, but they are not valid for a perfmap file.
The "Invalid file format" assert will result in an exception, but this would be
lost when Files.deleteIfExists() also throws a permissions exception. The way
around this is really ugly:
try {
run(new JMXExecutor(), "Compiler.perfmap " + path.toString(), path);
Files.deleteIfExists(path);
} catch (Throwable t) {
t.printStackTrace(System.out);
Files.deleteIfExists(path);
throw t;
}
So if run() passes, we do the deleteIfExists() and allow it to throw an
exception if needed (which would cause the test to appropriately fail with this
exception). If run() throws an exception, we catch it, print it, call
deleteIfExists(), and then rethrow the exception. If deleteIfExists() throws an
exception, then that is what the test will exit with, but at least we'll have
first printed the exception from run(), so it is not lost. However, if
deleteIfExists() doesn't throw an exception, we end up printing the exception
thrown by run() twice. Once in this added exception handler, and once by jtreg
on test exit. Can't say I like this approach, but I don't have a better
solution ATM.
The other choice is what we have now. Don't put deleteIfExists() in a finally
block, and don't bother deleting the file if run() throws an exception. The
main case where this becomes a problem is if the file is accessible, is written
to by the jcmd, but the contents for some reason don't pass the sanity test.
The test will appropriate fail, but it will also leave the perfmap file behind.
Maybe this is acceptable since it should never happen, and does help keep the
test code a lot simpler.
-
PR Comment: https://git.openjdk.org/jdk/pull/17992#issuecomment-1973983886
Integrated: 8327136: javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java fails on libgraal
On Fri, 1 Mar 2024 17:24:02 GMT, Doug Simon wrote: > To account for slightly slower compile times on libgraal + fastdebug + > `-Xcomp`, this PR increases a timeout in `NotifReconnectDeadlockTest.java` > from 2000 ms to 3000 ms. > With this change, the test now reliably passes. This pull request has now been integrated. Changeset: 8f0fb27d Author:Doug Simon URL: https://git.openjdk.org/jdk/commit/8f0fb27decec28f32e4d88341237189ba4a340fb Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8327136: javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java fails on libgraal Reviewed-by: never, kevinw - PR: https://git.openjdk.org/jdk/pull/18085
Re: RFR: 8327136: javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java fails on libgraal
On Fri, 1 Mar 2024 17:24:02 GMT, Doug Simon wrote: > To account for slightly slower compile times on libgraal + fastdebug + > `-Xcomp`, this PR increases a timeout in `NotifReconnectDeadlockTest.java` > from 2000 ms to 3000 ms. > With this change, the test now reliably passes. Thanks for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/18085#issuecomment-1973749075
Re: RFR: 8327136: javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java fails on libgraal
On Fri, 1 Mar 2024 17:24:02 GMT, Doug Simon wrote: > To account for slightly slower compile times on libgraal + fastdebug + > `-Xcomp`, this PR increases a timeout in `NotifReconnectDeadlockTest.java` > from 2000 ms to 3000 ms. > With this change, the test now reliably passes. Sounds good. One day, somebody among us will implement timeouts that take into account whether there are JVM options like -Xcomp or other factors that slow things down... 8-) - Marked as reviewed by kevinw (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18085#pullrequestreview-1911784742
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v8]
On Wed, 14 Feb 2024 21:13:56 GMT, Daniel D. Daugherty wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: added assert to get_pending_threads; added suggested coverage to >> test objmonusage003 > > CHECK_FOR_BAD_RESULTS and ADD_DELAYS_FOR_RACES flags and > their associated code paths are for temporary debugging only. @dcubed-ojdk @dholmes-ora Dan and David, I think, I've addressed all your comments. Do you have any other concerns? Can you approve it now? Does the RN look okay? - PR Comment: https://git.openjdk.org/jdk/pull/17680#issuecomment-1973668452
Re: RFR: 8327136: javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java fails on libgraal
On Fri, 1 Mar 2024 17:24:02 GMT, Doug Simon wrote: > To account for slightly slower compile times on libgraal + fastdebug + > `-Xcomp`, this PR increases a timeout in `NotifReconnectDeadlockTest.java` > from 2000 ms to 3000 ms. > With this change, the test now reliably passes. Looks good - Marked as reviewed by never (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18085#pullrequestreview-1911730736
RFR: 8327136: javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java fails on libgraal
To account for slightly slower compile times on libgraal + fastdebug + `-Xcomp`, this PR increases a timeout in `NotifReconnectDeadlockTest.java` from 2000 ms to 3000 ms. With this change, the test now reliably passes. - Commit messages: - adjust timeout in NotifReconnectDeadlockTest.java Changes: https://git.openjdk.org/jdk/pull/18085/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18085&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8327136 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18085.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18085/head:pull/18085 PR: https://git.openjdk.org/jdk/pull/18085
Re: RFR: 8226919: attach in linux hangs due to permission denied accessing /proc/pid/root
On Fri, 1 Mar 2024 09:31:47 GMT, Kevin Walls wrote: >> @slovdahl - Apologies for adding a comment to a closed Pull Request, but I >> happened on https://bugs.openjdk.org/browse/JDK-8307977 via the earlier >> https://bugs.openjdk.org/browse/JDK-8179498 after researching >> "AttachNotSupportedException: Unable to open socket file" and >> troubleshooting our own OpenJDK 17 jcmd setup on top of containers and >> Kubernetes. Reading the code changes and discussion here, I'm concerned >> that this change, which I understand is not yet in OpenJDK 17, might cause a >> regression with our setup. >> >> We're running jcmd (OpenJDK build 17.0.10+7-LTS) and the target JVM in two >> separate containers in a Kubernetes pod. The target JVM container is >> already running, and then we use `kubectl debug --target=...` to start a >> Kubernetes debug container with jcmd that targets the first container. >> Given the `--target` option, they share the same Linux process namespace >> (both think the target JVM is PID 1). But since they are separate >> containers, they see different root filesystems (jcmd container sees the >> target JVM tmpdir under /proc/1/root/tmp but has its own distinct /tmp >> directory). >> >> I believe the attach file and socket file paths then work like this in >> OpenJDK 17: >> * jcmd creates the .attach_pid1 attach file without issues using /proc/1/cwd >> * Target JVM finds the .attach_pid1 attach file in its cwd. >> * Target JVM creates the .java_pid1 socket file in its tmpdir /tmp >> * jcmd finds the .java_pid1 socket file in /proc/1/root/tmp >> >> I think this scenario with a Kubernetes debug container may be a little >> different from other Docker container scenarios because these are two >> different containers with _different root filesystems_ but _the same Linux >> process namespace_. So jcmd using `/proc//root` is necessary to find >> the socket file, even though jcmd and the target JVM both agree the PID is >> the same (1). A similar scenario with just Docker Engine is described at >> [docker container run - Example, join another container's PID >> namespace](https://docs.docker.com/reference/cli/docker/container/run/#example-join-another-containers-pid-namespace). >> >> If I understand the code change for this PR, I think it will change the >> behavior in this scenario, because `findSocketFile` will have `pid == >> ns_pid`, and now will use /tmp instead of `/proc//root/tmp`, based on >> `findTargetProcessTmpDirectory`. >> >> We are lucky currently that the only place the current OpenJDK 17 code >> checks `pid == ns_pid` is the `createAttachFile` catch block that ru... > > Logged https://bugs.openjdk.org/browse/JDK-8327114 for investigation. Thanks > @jdoylei ! @kevinjwalls - Perfect, thank you for opening the JBS bug! - PR Comment: https://git.openjdk.org/jdk/pull/17628#issuecomment-1973380664
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v21]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request incrementally with one additional
commit since the last revision:
rename after merge: jvmti_common.h to jvmti_common.hpp
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17680/files
- new: https://git.openjdk.org/jdk/pull/17680/files/f1a97f51..b449f04a
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=17680&range=20
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=17680&range=19-20
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly [v4]
> The implementation of the JVM TI `GetCurrentContendedMonitor()` does not > match the spec. It can sometimes return an incorrect information about the > contended monitor. Such a behavior does not match the function spec. > With this update the `GetCurrentContendedMonitor()` is returning the monitor > only when the specified thread is waiting to enter or re-enter the monitor, > and the monitor is not returned when the specified thread is waiting in the > `java.lang.Object.wait` to be notified. > > The implementations of the JDWP `ThreadReference.CurrentContendedMonitor` > command and JDI `ThreadReference.currentContendedMonitor()` method are based > and depend on this JVMTI function. The JDWP command and the JDI method were > specified incorrectly and had incorrect behavior. The fix slightly corrects > both the JDWP and JDI specs. The JDWP and JDI implementations have been also > fixed because they use this JVM TI update. Please, see and review the related > CSR and Release-Note. > > CSR: [8326024](https://bugs.openjdk.org/browse/JDK-8326024): JVM TI > GetCurrentContendedMonitor is implemented incorrectly > RN: [8326038](https://bugs.openjdk.org/browse/JDK-8326038): Release Note: > JVM TI GetCurrentContendedMonitor is implemented incorrectly > > Testing: > - tested with the mach5 tiers 1-6 Serguei Spitsyn 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 four additional commits since the last revision: - Merge - Merge - fix specs: JDWP ThreadReference.CurrentContendedMonitor command, JDI ThreadReference.currentContendedMonitor method - 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly - Changes: - all: https://git.openjdk.org/jdk/pull/17944/files - new: https://git.openjdk.org/jdk/pull/17944/files/a4e8c5b5..1a3d1c19 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17944&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17944&range=02-03 Stats: 2399 lines in 780 files changed: 475 ins; 436 del; 1488 mod Patch: https://git.openjdk.org/jdk/pull/17944.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17944/head:pull/17944 PR: https://git.openjdk.org/jdk/pull/17944
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v20]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 22 commits:
- Merge
- review: update comment in threads.hpp
- fix deadlock with carrier threads starvation in ObjectMonitorUsage test
- resolve merge conflict for deleted file objmonusage003.cpp
- fix a typo in libObjectMonitorUsage.cpp
- fix potential sync gap in the test ObjectMonitorUsage
- improve ObjectMonitorUsage test native agent output
- improved the ObjectMonitorUsage test to make it more elegant
- review: remove test objmonusage003; improve test ObjectMonitorUsage
- review: addressed minor issue with use of []; corrected the test desctiption
- ... and 12 more: https://git.openjdk.org/jdk/compare/e85265ab...f1a97f51
-
Changes: https://git.openjdk.org/jdk/pull/17680/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17680&range=19
Stats: 1123 lines in 15 files changed: 686 ins; 389 del; 48 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly [v3]
On Thu, 29 Feb 2024 04:17:09 GMT, Serguei Spitsyn wrote: >> The implementation of the JVM TI `GetCurrentContendedMonitor()` does not >> match the spec. It can sometimes return an incorrect information about the >> contended monitor. Such a behavior does not match the function spec. >> With this update the `GetCurrentContendedMonitor()` is returning the monitor >> only when the specified thread is waiting to enter or re-enter the monitor, >> and the monitor is not returned when the specified thread is waiting in the >> `java.lang.Object.wait` to be notified. >> >> The implementations of the JDWP `ThreadReference.CurrentContendedMonitor` >> command and JDI `ThreadReference.currentContendedMonitor()` method are based >> and depend on this JVMTI function. The JDWP command and the JDI method were >> specified incorrectly and had incorrect behavior. The fix slightly corrects >> both the JDWP and JDI specs. The JDWP and JDI implementations have been also >> fixed because they use this JVM TI update. Please, see and review the >> related CSR and Release-Note. >> >> CSR: [8326024](https://bugs.openjdk.org/browse/JDK-8326024): JVM TI >> GetCurrentContendedMonitor is implemented incorrectly >> RN: [8326038](https://bugs.openjdk.org/browse/JDK-8326038): Release Note: >> JVM TI GetCurrentContendedMonitor is implemented incorrectly >> >> Testing: >> - tested with the mach5 tiers 1-6 > > Serguei Spitsyn 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 three additional > commits since the last revision: > > - Merge > - fix specs: JDWP ThreadReference.CurrentContendedMonitor command, JDI > ThreadReference.currentContendedMonitor method > - 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly PING!! Need a couple of reviews, please. - PR Comment: https://git.openjdk.org/jdk/pull/17944#issuecomment-1972989702
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v19]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request incrementally with two additional
commits since the last revision:
- review: update comment in threads.hpp
- fix deadlock with carrier threads starvation in ObjectMonitorUsage test
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17680/files
- new: https://git.openjdk.org/jdk/pull/17680/files/06711644..7244d349
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=17680&range=18
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=17680&range=17-18
Stats: 4 lines in 2 files changed: 2 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8326666: Remove the Java Management Extension (JMX) Subject Delegation feature
On Tue, 27 Feb 2024 10:44:20 GMT, Kevin Walls wrote: > The deprecated Subject Delegation feature in JMX will be removed. > > This was marked in JDK 21 as deprecated for removal (JDK-8298966). This covers the public interface to the feature, and tests. Locally the feature is refused during a call to getMBeanServerConnection(with a non-null delegationSubject). Remotely, `javax.management.remote.rmi.RMIConnectionImpl` has methods which accept a Subject parameter for Subject Delegation. This cannot now be non-null in local usage, but an RMI Connection from an older JDK can invoke these methods with a non-null delegationSubject. Such access will be recognised and an UnsupportedOperationException will be thrown (which the client will see). RMIConnectionImpl can be simplified, and the class SubjectDelegator removed. JMXSubjectDomainCombiner I would like to remove separately after this is done. - PR Comment: https://git.openjdk.org/jdk/pull/18025#issuecomment-1972845693
RFR: 8326666: Remove the Java Management Extension (JMX) Subject Delegation feature
The deprecated Subject Delegation feature in JMX will be removed. This was marked in JDK 21 as deprecated for removal (JDK-8298966). - Commit messages: - JMXConnector to throw UnsupportedOperationException - RMIConnectionImpl to throw when RMI calls invoke with delegatedSubject. Removal of SubjectDelegator class. - Use default method. - Remove delegationSubject from private class RemoteMBeanServerConnection - 832: Remove the Java Management Extension (JMX) Subject Delegation feature Changes: https://git.openjdk.org/jdk/pull/18025/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18025&range=00 Issue: https://bugs.openjdk.org/browse/JDK-832 Stats: 1642 lines in 29 files changed: 18 ins; 1547 del; 77 mod Patch: https://git.openjdk.org/jdk/pull/18025.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18025/head:pull/18025 PR: https://git.openjdk.org/jdk/pull/18025
Re: RFR: 8226919: attach in linux hangs due to permission denied accessing /proc/pid/root
On Wed, 28 Feb 2024 20:02:39 GMT, jdoylei wrote: >> Thank you @kevinjwalls and @jerboaa for reviewing and guiding me through >> this process, this was a great as a first-time JDK contributor! >> >> One more question, can I do anything to help getting this backported to e.g. >> 21 and 17? > > @slovdahl - Apologies for adding a comment to a closed Pull Request, but I > happened on https://bugs.openjdk.org/browse/JDK-8307977 via the earlier > https://bugs.openjdk.org/browse/JDK-8179498 after researching > "AttachNotSupportedException: Unable to open socket file" and troubleshooting > our own OpenJDK 17 jcmd setup on top of containers and Kubernetes. Reading > the code changes and discussion here, I'm concerned that this change, which I > understand is not yet in OpenJDK 17, might cause a regression with our setup. > > We're running jcmd (OpenJDK build 17.0.10+7-LTS) and the target JVM in two > separate containers in a Kubernetes pod. The target JVM container is already > running, and then we use `kubectl debug --target=...` to start a Kubernetes > debug container with jcmd that targets the first container. Given the > `--target` option, they share the same Linux process namespace (both think > the target JVM is PID 1). But since they are separate containers, they see > different root filesystems (jcmd container sees the target JVM tmpdir under > /proc/1/root/tmp but has its own distinct /tmp directory). > > I believe the attach file and socket file paths then work like this in > OpenJDK 17: > * jcmd creates the .attach_pid1 attach file without issues using /proc/1/cwd > * Target JVM finds the .attach_pid1 attach file in its cwd. > * Target JVM creates the .java_pid1 socket file in its tmpdir /tmp > * jcmd finds the .java_pid1 socket file in /proc/1/root/tmp > > I think this scenario with a Kubernetes debug container may be a little > different from other Docker container scenarios because these are two > different containers with _different root filesystems_ but _the same Linux > process namespace_. So jcmd using `/proc//root` is necessary to find > the socket file, even though jcmd and the target JVM both agree the PID is > the same (1). A similar scenario with just Docker Engine is described at > [docker container run - Example, join another container's PID > namespace](https://docs.docker.com/reference/cli/docker/container/run/#example-join-another-containers-pid-namespace). > > If I understand the code change for this PR, I think it will change the > behavior in this scenario, because `findSocketFile` will have `pid == > ns_pid`, and now will use /tmp instead of `/proc//root/tmp`, based on > `findTargetProcessTmpDirectory`. > > We are lucky currently that the only place the current OpenJDK 17 code checks > `pid == ns_pid` is the `createAttachFile` catch block that runs if > `/proc//cwd/.attach... Logged https://bugs.openjdk.org/browse/JDK-8327114 for investigation. Thanks @jdoylei ! - PR Comment: https://git.openjdk.org/jdk/pull/17628#issuecomment-1972833655
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information [v3]
> Introduce the jcmd "VM.debug" to implement access to a useful set of the > established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...". > > Not recommended for live production use. Calling these "debug" utilities, > and not including them in the jcmd help output, is to remind us they are not > general customer-facing tools. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Test update omitted from previous commit. - Changes: - all: https://git.openjdk.org/jdk/pull/17655/files - new: https://git.openjdk.org/jdk/pull/17655/files/f57f7274..cacbca27 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17655&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17655&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17655.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17655/head:pull/17655 PR: https://git.openjdk.org/jdk/pull/17655
