Re: RFR: 8188784: javax/management/notification/BroadcasterSupportDeadlockTest.java - TEST FAILED: deadlock [v2]
On Tue, 16 Apr 2024 09:26:58 GMT, Kevin Walls wrote: >> This test incorrectly fails, although rarely, thinking its "thread 2" has >> deadlocked. >> A change of sleep will likely fix this, but there are other issues, so >> cleaning up the test a little. >> >> Remove the probe for the ManagementFactory class, to check we are on jdk5 or >> later. 8-) >> >> When sleeping, sleep 100, not 1ms, we don't need to spin fast and actually >> race with the other thread. >> >> We have a 1000 iteration loop, but don't seem to use it. We only check once >> then either return (pass), fail, or break (which is also fail). Use the >> loop to check for the status change, which is likely what was intended. >> >> Show the stackframes on all failures. > > Kevin Walls 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: > > - Remove System.exit calls > - Merge remote-tracking branch 'upstream/master' into > 8188784_BroadcasterSupportDeadlockTest > - 8188784: javax/management/notification/BroadcasterSupportDeadlockTest.java > - TEST FAILED: deadlock Thanks for the reviews! - PR Comment: https://git.openjdk.org/jdk/pull/18687#issuecomment-2058745011
Re: RFR: 8188784: javax/management/notification/BroadcasterSupportDeadlockTest.java - TEST FAILED: deadlock [v2]
On Tue, 16 Apr 2024 00:31:10 GMT, Leonid Mesnik wrote:
>> Kevin Walls 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:
>>
>> - Remove System.exit calls
>> - Merge remote-tracking branch 'upstream/master' into
>> 8188784_BroadcasterSupportDeadlockTest
>> - 8188784:
>> javax/management/notification/BroadcasterSupportDeadlockTest.java - TEST
>> FAILED: deadlock
>
> test/jdk/javax/management/notification/BroadcasterSupportDeadlockTest.java
> line 122:
>
>> 120: java.util.Map traces =
>> Thread.getAllStackTraces();
>> 121: showStackTrace("Thread 1", traces.get(t1));
>> 122: showStackTrace("Thread 2", traces.get(t2));
>
> Could you please replace System.exit() with throwing Exception. Other looks
> good.
OK sure.
FYI I count 137 System.exit calls in test/jdk/javax/management
That's after I take out the existing 3 that are in this test. 8-)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18687#discussion_r1567098465
Re: RFR: 8188784: javax/management/notification/BroadcasterSupportDeadlockTest.java - TEST FAILED: deadlock [v2]
> This test incorrectly fails, although rarely, thinking its "thread 2" has > deadlocked. > A change of sleep will likely fix this, but there are other issues, so > cleaning up the test a little. > > Remove the probe for the ManagementFactory class, to check we are on jdk5 or > later. 8-) > > When sleeping, sleep 100, not 1ms, we don't need to spin fast and actually > race with the other thread. > > We have a 1000 iteration loop, but don't seem to use it. We only check once > then either return (pass), fail, or break (which is also fail). Use the loop > to check for the status change, which is likely what was intended. > > Show the stackframes on all failures. Kevin Walls 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: - Remove System.exit calls - Merge remote-tracking branch 'upstream/master' into 8188784_BroadcasterSupportDeadlockTest - 8188784: javax/management/notification/BroadcasterSupportDeadlockTest.java - TEST FAILED: deadlock - Changes: - all: https://git.openjdk.org/jdk/pull/18687/files - new: https://git.openjdk.org/jdk/pull/18687/files/5237f1d8..a77b3c85 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18687=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18687=00-01 Stats: 16713 lines in 561 files changed: 8813 ins; 4016 del; 3884 mod Patch: https://git.openjdk.org/jdk/pull/18687.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18687/head:pull/18687 PR: https://git.openjdk.org/jdk/pull/18687
