Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v6]
On Wed, 12 Jun 2024 16:11:28 GMT, Kevin Walls wrote:
>> JMX uses APIs related to the Security Mananger which are deprecated. Use of
>> AccessControlContext will be removed when Security Manager is removed.
>>
>> Until then, updates are needed to not require setting
>> -Djava.security.manager=allow to use JMX authentication.
>
> Kevin Walls has updated the pull request incrementally with one additional
> commit since the last revision:
>
>Undo test policy updates
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
line 1445:
> 1443: } else {
> 1444: throw new PrivilegedActionException(e);
> 1445: }
I assume there no chance that `Exception e` may already be a
`PrivilegedActionException` here. You coud avoid casts by using instanceof
patterns.
Suggestion:
if (e instanceof RuntimeException rte) {
throw rte;
} else {
throw new PrivilegedActionException(e);
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1636791497
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v4]
On Wed, 12 Jun 2024 14:44:56 GMT, Kevin Walls wrote: >> I think Daniel is right, can you remove this permission and paste in the >> debug output to see where this is happening? > > Hmm I may have fixed that since changing the policy files, as I'm not seeing > the problem without that AuthPermission any more. Am just retesting > everything before updating this... (Same with other policy files in which the permission was added of course) - PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1636634416
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v4]
On Wed, 12 Jun 2024 14:01:40 GMT, Kevin Walls wrote: >> JMX uses APIs related to the Security Mananger which are deprecated. Use of >> AccessControlContext will be removed when Security Manager is removed. >> >> Until then, updates are needed to not require setting >> -Djava.security.manager=allow to use JMX authentication. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > udpates test/jdk/javax/management/remote/mandatory/notif/policy.negative line 7: > 5: permission javax.management.MBeanPermission "[domain:type=NB,name=2]", > "addNotificationListener"; > 6: permission javax.management.MBeanPermission "*", > "removeNotificationListener"; > 7: permission javax.security.auth.AuthPermission "doAs"; I suspect that this means a doPrivileged is missing somewhere. We should not require the application to posess new permissions. - PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1636573141
Integrated: 8333270: HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests fail with "Unexpected reference" if timeoutFactor is less than 1/3
On Fri, 31 May 2024 14:55:57 GMT, Daniel Fuchs wrote: > HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests verify that > loggers are GC'ed (or not GC'ed) after a reset or an update. For that they > poll a ReferenceQueue in a loop. The number of iteration is adjusted > according to the jtreg timeout factor. However, the logic in the test did not > expect that the timeout might be less than 1. > > This fix does two things: > > 1. fix the adjustCount logic - so that the number of iteration can only be > increased > 2. use remove(timeout) instead of poll() in order to minimize the waiting > time. This pull request has now been integrated. Changeset: d02cb742 Author:Daniel Fuchs URL: https://git.openjdk.org/jdk/commit/d02cb742f79e88c6438ca58a6357fe432fb286cb Stats: 16 lines in 2 files changed: 0 ins; 2 del; 14 mod 8333270: HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests fail with "Unexpected reference" if timeoutFactor is less than 1/3 Reviewed-by: jpai - PR: https://git.openjdk.org/jdk/pull/19503
Re: RFR: 8333270: HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests fail with "Unexpected reference" if timeoutFactor is less than 1/3 [v2]
On Sat, 1 Jun 2024 05:20:24 GMT, Jaikiran Pai wrote:
>> Daniel Fuchs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Review feedback: use Reference::refersTo consistently
>
> test/jdk/java/util/logging/LogManager/Configuration/updateConfiguration/HandlersOnComplexResetUpdate.java
> line 219:
>
>> 217: }
>> 218: WeakReference barRef = new
>> WeakReference<>(Logger.getLogger("com.bar"), queue);
>> 219: if (!barRef.refersTo(barChild.getParent())) {
>
> Hello Daniel, since `refersTo()` is the preferred API in cases like this,
> should this same change be done in the other `HandlersOnComplexUpdate` test
> that's being updated in this PR?
Good point. Updated.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19503#discussion_r1626196539
Re: RFR: 8333270: HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests fail with "Unexpected reference" if timeoutFactor is less than 1/3 [v2]
> HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests verify that > loggers are GC'ed (or not GC'ed) after a reset or an update. For that they > poll a ReferenceQueue in a loop. The number of iteration is adjusted > according to the jtreg timeout factor. However, the logic in the test did not > expect that the timeout might be less than 1. > > This fix does two things: > > 1. fix the adjustCount logic - so that the number of iteration can only be > increased > 2. use remove(timeout) instead of poll() in order to minimize the waiting > time. Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Review feedback: use Reference::refersTo consistently - Changes: - all: https://git.openjdk.org/jdk/pull/19503/files - new: https://git.openjdk.org/jdk/pull/19503/files/be5a4d82..75576a8d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19503=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19503=00-01 Stats: 5 lines in 2 files changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/19503.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19503/head:pull/19503 PR: https://git.openjdk.org/jdk/pull/19503
RFR: 8333270: HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests fail with "Unexpected reference" if timeoutFactor is less than 1/3
HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests verify that loggers are GC'ed (or not GC'ed) after a reset or an update. For that they poll a ReferenceQueue in a loop. The number of iteration is adjusted according to the jtreg timeout factor. However, the logic in the test did not expect that the timeout might be less than 1. This fix does two things: 1. fix the adjustCount logic - so that the number of iteration can only be increased 2. use remove(timeout) instead of poll() in order to minimize the waiting time. - Commit messages: - 8333270 Changes: https://git.openjdk.org/jdk/pull/19503/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19503=00 Issue: https://bugs.openjdk.org/browse/JDK-8333270 Stats: 11 lines in 2 files changed: 0 ins; 2 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/19503.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19503/head:pull/19503 PR: https://git.openjdk.org/jdk/pull/19503
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base` [v2]
On Tue, 28 May 2024 22:31:15 GMT, Jonathan Gibbons wrote: >> With the advent of JEP 467, `///` comments may be treated as documentation >> comments, and may be subject to the recently new `javac` warning about >> "dangling doc comments" in unexpected places. >> >> In keeping with the policy to keep the `java.base` module free of all >> `javac` warnings, this patch proposes edits to existing uses of `///`. >> >> There are two dominant policies in the proposed changes. >> 1. A long horizontal line of `/` is replaced by `//-` >> 2. A long vertical series of lines beginning `///` is replaced by lines >> beginning `//|`. >> >> As with all style changes, I have also tried to honor local usage, for >> consistency. >> >> In one place, a pair of comments appeared to contain directives >> (`CLOVER:ON`, `CLOVER:OFF`). I investigated the use of such comments to >> determine that the exact form of the comment prefix was not significant. >> (Phew!) >> >> >> (This PR is informally blocked by JEP 467). > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > incorporate review comments I like `//---` ; +1! - PR Comment: https://git.openjdk.org/jdk/pull/19130#issuecomment-2137452920
Re: RFR: 8331670: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal [v3]
On Tue, 21 May 2024 07:26:17 GMT, Alan Bateman wrote: >> This is the implementation changes for JEP 471. >> >> The methods in sun.misc.Unsafe for on-heap and off-heap access are >> deprecated for removal. This means a removal warning at compile time. No >> methods have been removed. A deprecated message is added to each of the >> methods but unlikely to be seen as the JDK does not generate or publish the >> API docs for this class. >> >> A new command line option --sun-misc-unsafe-memory-access=$value is >> introduced to allow or deny access to these methods. The default proposed >> for JDK 23 is "allow" so no change in behavior compared to JDK 22 or >> previous releases. >> >> A new test is added to test the command line option settings. The existing >> micros for FFM that use Unsafe are updated to suppress the removal warning >> at compile time. A new micro is introduced with a small sample of methods to >> ensure the changes doesn't cause any perf regressions ([sample >> results](https://cr.openjdk.org/~alanb/8331670-results.txt)). >> >> For now, the changes include the update to the man page for the "java" >> command. It might be that this has to be separated out so that it goes with >> other updates in the release. > > Alan Bateman has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 10 commits: > > - Merge > - Add removal to DISABLED_WARNINGS >Fail at startup if bad value specified to option > - Merge > - Update man page > - Update man page > - Fix comment > - Merge > - Merge > - Whitespace > - Initial commit src/jdk.unsupported/share/classes/sun/misc/Unsafe.java line 1750: > 1748: } > 1749: > 1750: // Instructure for --sun-misc-unsafe-memory-access= command > line option. Suggestion: // Infrastructure for --sun-misc-unsafe-memory-access= command line option. - PR Review Comment: https://git.openjdk.org/jdk/pull/19174#discussion_r1613644783
Re: RFR: 8331987: Enhance stacktrace clarity for CompletableFuture CancellationException [v2]
On Mon, 13 May 2024 23:59:17 GMT, Viktor Klang wrote: >> This change adds wrapping of the CancellationException produced by >> CompletableFuture::get() and CompletableFuture::join() to add more >> diagnostic information and align better with FutureTask. >> >> Running the sample code from the JBS issue in JShell will produce the >> following: >> >> >> jshell> java.util.concurrent.CancellationException: get >> at >> java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:392) >> at >> java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2073) >> at REPL.$JShell$18.m2($JShell$18.java:10) >> at REPL.$JShell$17.m1($JShell$17.java:8) >> at REPL.$JShell$16B.lambda$main$0($JShell$16B.java:8) >> at java.base/java.lang.Thread.run(Thread.java:1575) >> Caused by: java.util.concurrent.CancellationException >> at >> java.base/java.util.concurrent.CompletableFuture.cancel(CompletableFuture.java:2510) >> at REPL.$JShell$16B.lambda$main$1($JShell$16B.java:11) >> ... 1 more > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Adding CancellationException detail message to CompletableFuture LGTM - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19219#pullrequestreview-2057484574
Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory
On Mon, 13 May 2024 17:12:33 GMT, Raffaello Giulietti wrote: >> Not sure if that's an issue - but if you wanted/needed to delay the loading >> of those random generator classes that will not be used until something >> actually wants to use them, you could consider using a `Supplier> extends RandomGenerator>>` or a `Supplier` for the >> `FACTORY_MAP` values? > > @dfuch You mean not loading the whole batch but only individual classes, as > need arises? yes - I do not know if loading all the classes in one batch could be an issue at startup (I'd expect Random to be loaded early) - but if it proves to be, then using a Supplier to load them on demand might do the trick. If creating Random or SecureRandom does not trigger this code - and if those classes are not loaded at startup - then maybe that's a non issue and you can just ignore my comment. - PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1599847940
Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory
On Mon, 13 May 2024 13:21:23 GMT, Raffaello Giulietti wrote: >> A followup PR is fine. I think once this PR which addresses the API aspects >> (like removal of ServiceLoader and then updates to the create() method >> javadoc) is integrated, then the subsequent PR can just be all internal >> implementation changes like the proposed removal of reflection. > > Fine with me. Not sure if that's an issue - but if you wanted/needed to delay the loading of those random generator classes that will not be used until something actually wants to use them, you could consider using a `Supplier>` or a `Supplier` for the `FACTORY_MAP` values? - PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1598616209
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]
On Mon, 13 May 2024 11:47:38 GMT, Maurizio Cimadamore wrote: >> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting >> the use of JNI in the following ways: >> >> * `System::load` and `System::loadLibrary` are now restricted methods >> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods >> * binding a JNI `native` method declaration to a native implementation is >> now considered a restricted operation >> >> This PR slightly changes the way in which the JDK deals with restricted >> methods, even for FFM API calls. In Java 22, the single >> `--enable-native-access` was used both to specify a set of modules for which >> native access should be allowed *and* to specify whether illegal native >> access (that is, native access occurring from a module not specified by >> `--enable-native-access`) should be treated as an error or a warning. More >> specifically, an error is only issued if the `--enable-native-access flag` >> is used at least once. >> >> Here, a new flag is introduced, namely >> `illegal-native-access=allow/warn/deny`, which is used to specify what >> should happen when access to a restricted method and/or functionality is >> found outside the set of modules specified with `--enable-native-access`. >> The default policy is `warn`, but users can select `allow` to suppress the >> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This >> aligns the treatment of restricted methods with other mechanisms, such as >> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`. >> >> Some changes were required in the package-info javadoc for >> `java.lang.foreign`, to reflect the changes in the command line flags >> described above. > > Maurizio Cimadamore has updated the pull request incrementally with three > additional commits since the last revision: > > - Fix another typo > - Fix typo > - Add more comments Changes to jdk.net and jdk.sctp look ok. - PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2107695217
Re: RFR: 8331876: JFR: Move file read and write events to java.base [v3]
On Thu, 9 May 2024 11:19:14 GMT, Erik Gahlin wrote: >> Hi, >> >> Could I have a review of a change that moves the jdk.FileRead and >> jdk.FileWrite events to java.base to remove the use of the ASM >> instrumentation. >> >> Testing: jdk/jdk/jfr >> >> Thanks >> Erik > > Erik Gahlin has updated the pull request incrementally with one additional > commit since the last revision: > > Move methods src/java.base/share/classes/java/io/FileInputStream.java line 63: > 61: private static final int DEFAULT_BUFFER_SIZE = 8192; > 62: > 63: // Flag that determines if file reads should be traced by JFR It could be good to also note what will modify this flag - here and in other classes. I'm going to guess that the purpose of this flag is purely performance, to avoid even loading the event class, `FileReadEvent` here, during startup/bootstrap and when JFR is not enabled, as read and write methods are highly performance sensitive? Otherwise the flag could live in the event class itself? Is it substantially faster to check this flag compared to `FileReadEvent.enabled()`? src/java.base/share/classes/jdk/internal/event/JFRTracing.java line 51: > 49: field.setAccessible(true); > 50: field.setBoolean(null, true); > 51: } Using reflection with `Field` seems expedient - a more modern way could be to use `VarHandle` but I guess it would require more setup to obtain a `Lookup` with the proper capabilities? - PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1595525764 PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1595530833
Re: RFR: 8331514: Tests should not use the "Classpath" exception form of the legal header
On Thu, 2 May 2024 04:29:26 GMT, Jaikiran Pai wrote: > Can I please get a review of this copyright text only change which removes > the "Classpath" exception from the > `test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java` test file and > thus addresses https://bugs.openjdk.org/browse/JDK-8331514? LGTM - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19046#pullrequestreview-2035163452
Re: RFR: 8330178: Clean up non-standard use of /** comments in `java.base`
On Thu, 18 Apr 2024 20:44:00 GMT, Jonathan Gibbons wrote: > Please review a set of updates to clean up use of `/**` comments in the > vicinity of declarations. > > There are various categories of update: > > * "Box comments" beginning with `/**` > * Misplaced doc comments before package or import statements > * Misplaced doc comments after the annotations for a declaration > * Dangling `/**` comments relating to a group of declarations, separate from > the doc comments for each of those declarations > * Use of `/**` for the legal header at or near the top of the file > > In one case, two doc comments before a declaration were merged, which fixes a > bug/omission in the documented serialized form. Changes to networking code looks good. I didn't spot any issue with the rest but I'm usually not a reviewer there. - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18846#pullrequestreview-2011186056
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v5]
On Wed, 17 Apr 2024 01:34:07 GMT, Tim Prinzing wrote: >> Currently the JFR event FileForceEvent is generated by instrumenting the >> sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer >> mirror events with static methods. >> >> Added the event at jdk.internal.event.FileForceEvent, and changed >> jdk.jfr.events.FileForceEvent to be a mirror event. >> >> Updated FileChannelImpl to use the jdk internal event static methods, and >> removed the force() method from FileChannelImplInstrumentor. >> >> Uses the existing tests. > > Tim Prinzing has updated the pull request incrementally with one additional > commit since the last revision: > > test file local to test Sorry - just noticed this comment has been pending for a few days... src/jdk.jfr/share/classes/jdk/jfr/internal/instrument/JDKEvents.java line 66: > 64: FileWriteEvent.class, > 65: SocketReadEvent.class, > 66: SocketWriteEvent.class, I'm guessing that this change which remove these two event classes is a drive-by-cleanup that should actually have been done with some previous fix in this area? Just wanted to double check it was intended as it doesn't seem to be related to file events. - PR Review: https://git.openjdk.org/jdk/pull/18542#pullrequestreview-2006152864 PR Review Comment: https://git.openjdk.org/jdk/pull/18542#discussion_r1568907662
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v2]
On Mon, 15 Apr 2024 20:41:10 GMT, Tim Prinzing wrote:
>> test/jdk/jdk/jfr/event/io/TestAsynchronousFileChannelEvents.java line 50:
>>
>>> 48:
>>> 49: public static void main(String[] args) throws Throwable {
>>> 50: File blah = File.createTempFile("blah", null);
>>
>> Can you change this to use the tests's scratch rather that /tmp, meaning
>> `Files.createTempFile(Path.of("."), "blah", "jfr")`. That way the file is
>> available in the event that the test fails.
>
> I'm not sure what you mean about the recording. The file is the
> AsynchronousFileChannel under test and does not contain the event recording.
It's anyway better to create temporary files in the test scratch directory
rather than in /tmp. This way the temp files get cleaned up with the test, and
there less chances of conflicts if several tests are running concurrently (and
less chances on slowly filling up /tmp onthe machine if clean up actions fail)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18542#discussion_r1567596755
Re: RFR: 8310994: Add JFR event for selection operations [v3]
On Thu, 11 Apr 2024 16:08:31 GMT, Alan Bateman wrote: >> We should probably find a way to not emit the event if n == 0 and the >> operation was interrupted by `Selector.wakeUp`. Since we have another issue >> logged to emit a spin event, I wonder if we should only commit the event >> here if `n != 0`? The case where n == 0 would be handled by the spin event >> (added later) >> @AlanBateman what do you think? > > I think it's okay for now. If there is another phase of this work to help > diagnose spinning issues then it will need to re-visited. I'm very concerned > about the possible changes for that second phase, but this first phase of > instrumentation is not disruptive. OK. I am a little concerned about how often this event will be fired when using the HttpClient - given that it's enabled by default. Idle connections sitting in the pool will fire it at least once per connection every 1500ms. That may not be too bad. - PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1561313983
Re: RFR: 8310994: Add JFR event for selection operations [v3]
On Mon, 26 Feb 2024 17:40:59 GMT, Alan Bateman wrote: >> Is n == 0 intended to detect a spinning condition where the selector goes >> back into select when the event has not been handled? >> >> In that case should we still emit an event if a timeout is present and the >> duration is greater than the timeout? For instance, in the HttpClient, we >> have a selector thread that will wake up at regular interval - we set up a >> timeout which is the min between the next expected timer event and 1500ms. >> So that could potentially fire an event every 1500ms if for instance the >> connection threshold is less than 1500ms and the connection is idle. >> >> Maybe that's OK - and maybe in that case the onus is on the user to set a >> threshold greater than 1500ms? >> >> Or should it be ((n == 0 && durationToMillis(duration) < >> timeoutToMillis(timeout)) || ...)) (duration and timeout probably are not in >> the same unit of time) - also if shouldCommit return false will the event >> actually be emitted down the road? Because if not then the ( n== 0 || ...) >> won't work. > >> Maybe that's OK - and maybe in that case the onus is on the user to set a >> threshold greater than 1500ms? > > The threshold is 20ms so these timed-select ops in the HTTP client will > record an event when they timeout. We should probably find a way to not emit the event if n == 0 and the operation was interrupted by `Selector.wakeUp`. Since we have another issue logged to emit a spin event, I wonder if we should only commit the event here if `n != 0`? The case where n == 0 would be handled by the spin event (added later) @AlanBateman what do you think? - PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1560693482
Result: New Core Libraries Group Member: Per-Ake Minborg
Hi, The vote for Per-Ake Minborg (pminborg) [1] is now closed. Yes: 11 Veto: 0 Abstain: 0 According to the Bylaws definition of Lazy Consensus, this is sufficient to approve the nomination. best regards, -- daniel [1] https://mail.openjdk.org/pipermail/core-libs-dev/2024-March/120617.html
Re: RFR: 8329013: StackOverflowError when starting Apache Tomcat with signed jar [v2]
On Tue, 2 Apr 2024 14:59:33 GMT, Sean Coffey wrote: >> Calling extra logging calls during initialization of Logger libraries can >> cause recursion leading to StackOverflowError >> This patch adds logic to the EventHelper class to detect recursion and >> prevent it. > > Sean Coffey has updated the pull request incrementally with one additional > commit since the last revision: > > incorporate testcase feedback from Daniel Marked as reviewed by dfuchs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18534#pullrequestreview-1974475681
Re: RFR: 8329013: StackOverflowError when starting Apache Tomcat with signed jar [v2]
On Tue, 2 Apr 2024 13:38:00 GMT, Daniel Fuchs wrote: >> Sean Coffey has updated the pull request incrementally with one additional >> commit since the last revision: >> >> incorporate testcase feedback from Daniel > > test/jdk/jdk/security/logging/RecursiveEventHelper.java line 56: > >> 54: // a recursive call (via EventHelper.isLoggingSecurity) back into >> 55: // logger API >> 56: EventHelper.isLoggingSecurity(); > > As an additional check you could set a static volatile boolean to true here > and double check that it's been set at the end of the main method, In fact it doesn't need to be volatile since it all happens in the main thread - but that's not a big issue. - PR Review Comment: https://git.openjdk.org/jdk/pull/18534#discussion_r1548280946
Re: RFR: 8329013: StackOverflowError when starting Apache Tomcat with signed jar
On Thu, 28 Mar 2024 15:55:12 GMT, Sean Coffey wrote: > Calling extra logging calls during initialization of Logger libraries can > cause recursion leading to StackOverflowError > This patch adds logic to the EventHelper class to detect recursion and > prevent it. The code changes LGTM. Some comments on the test. test/jdk/jdk/security/logging/RecursiveEventHelper.java line 56: > 54: // a recursive call (via EventHelper.isLoggingSecurity) back into > 55: // logger API > 56: EventHelper.isLoggingSecurity(); As an additional check you could set a static volatile boolean to true here and double check that it's been set at the end of the main method, test/jdk/jdk/security/logging/RecursiveEventHelper.java line 59: > 57: return super.getProperty(p); > 58: } > 59: } add newline? - PR Review: https://git.openjdk.org/jdk/pull/18534#pullrequestreview-1973842520 PR Review Comment: https://git.openjdk.org/jdk/pull/18534#discussion_r1547904127 PR Review Comment: https://git.openjdk.org/jdk/pull/18534#discussion_r1547897811
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v9]
On Wed, 20 Mar 2024 21:19:38 GMT, Christoph Langer wrote: >> During analysing a customer case I figured out that we have an inconsistency >> between documentation and actual behavior in class >> com.sun.jndi.ldap.Connection. The [method documentation of >> com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) >> states: "If a timeout is supplied but unconnected sockets are not supported >> then the timeout is ignored and a connected socket is created." >> >> This, however does not happen. If a SocketFactory would not support >> unconnected sockets, it would likely throw a SocketException in >> [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). >> And since [the >> code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) >> does not check for this behavior, a connection with timeout value through a >> SocketFactory that does not support unconnected sockets would simply fail >> with an IOException. >> >> So we should either make the code adhere to what is documented or adapt the >> documentation to the actual behavior. >> >> I hereby try to fix the connect coding. Alternatively, we could also adapt >> the description - I have no strong opinion. What do the experts suggest? > > Christoph Langer 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 14 additional > commits since the last revision: > > - Review suggestions Aleksei > - Merge branch 'master' into JDK-8325579 > - Update module-info text > - Merge branch 'master' into JDK-8325579 > - Indentation > - Merge branch 'master' into JDK-8325579 > - Review feedback > - Rename back to LdapSSLHandshakeFailureTest to ease reviewing > - Merge branch 'master' into JDK-8325579 > - Typo > - ... and 4 more: https://git.openjdk.org/jdk/compare/b817e52b...8fdc039c Latest changes LGTM - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17797#pullrequestreview-1951411585
CFV: New Core Libraries Group Member: Per-Ake Minborg
Hi, I hereby nominate Per-Ake Minborg (pminborg) [1] to Membership in the Core Libraries Group [4]. Per-Ake is an OpenJDK Reviewer, a committer in the Leyden and Panama projects, and a member of Oracle’s Java Core Libraries team. Per-Ake has been actively participating in the development of the JDK and Panama projects for several years, and is one of the co-author of the Implementation of Foreign Function and Memory API (Third Preview) [2]. His contributions include more than 80 commits in the JDK [3] Votes are due by 16:00 UTC on April 2, 2024. Only current Members of the Core Libraries Group [4] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list. For Lazy Consensus voting instructions, see [5]. best regards, -- daniel [1] https://openjdk.org/census#pminborg [2] https://github.com/openjdk/jdk/commit/cbccc4c8172797ea2f1b7c301d00add3f517546d [3] https://github.com/search?q=repo%3Aopenjdk%2Fjdk+author%3Aminborg=commits [4] https://openjdk.org/census#core-libs [5] https://openjdk.org/groups/#member-vote
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v8]
On Thu, 14 Mar 2024 21:59:54 GMT, Christoph Langer wrote: >> During analysing a customer case I figured out that we have an inconsistency >> between documentation and actual behavior in class >> com.sun.jndi.ldap.Connection. The [method documentation of >> com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) >> states: "If a timeout is supplied but unconnected sockets are not supported >> then the timeout is ignored and a connected socket is created." >> >> This, however does not happen. If a SocketFactory would not support >> unconnected sockets, it would likely throw a SocketException in >> [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). >> And since [the >> code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) >> does not check for this behavior, a connection with timeout value through a >> SocketFactory that does not support unconnected sockets would simply fail >> with an IOException. >> >> So we should either make the code adhere to what is documented or adapt the >> documentation to the actual behavior. >> >> I hereby try to fix the connect coding. Alternatively, we could also adapt >> the description - I have no strong opinion. What do the experts suggest? > > Christoph Langer 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 12 additional > commits since the last revision: > > - Update module-info text > - Merge branch 'master' into JDK-8325579 > - Indentation > - Merge branch 'master' into JDK-8325579 > - Review feedback > - Rename back to LdapSSLHandshakeFailureTest to ease reviewing > - Merge branch 'master' into JDK-8325579 > - Typo > - Merge branch 'master' into JDK-8325579 > - Rename test and refine comment > - ... and 2 more: https://git.openjdk.org/jdk/compare/5d79093e...10271159 LGTM. I haven't looked at the updated test too closely. - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17797#pullrequestreview-1938532665
Re: RFR: 8328066: WhiteBoxResizeTest failure on linux-x86: Could not reserve enough space for 2097152KB object heap
On Thu, 14 Mar 2024 04:11:14 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only change which proposes to address > https://bugs.openjdk.org/browse/JDK-8328066? > > The test launches a JVM with 2G heap (`-Xmx2G`) and as noted in that issue, > the failure was observed on linux-86 instance on a GitHub jobs run. > > The commit in this PR proposes to add relevant `@requires` so that the test > is only run on 64 bit VM and expects the `os.maxMemory` to be > 2G. > > The change has been tested on a linux-x86 run in GitHub actions job, plus > even on local and CI 64 bit VM environments. No failures have been noticed. LGTM. I see that some other tests have things like: * @requires vm.bits == "64" & os.maxMemory > 4G Not sure what is the preferred form in such cases: two `@requires` or a single one? - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18290#pullrequestreview-1936420903
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]
On Tue, 5 Mar 2024 19:53:46 GMT, Weijun Wang wrote: >> Subject is stored in the RMIConnectionImpl: >> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java >> >> (That is complicated by SubjectDelegation, which we deprecated for removal. >> I have the PR out to remove it: >> https://github.com/openjdk/jdk/pull/18025 ) >> >> makeClient in RMIJRMPServerImpl creates RMIConnectionImpl >> >> ..and RMIServerImpl.java has a doNewClient method calling that. This is >> what takes a Credentials Object and deals withJMXAuthenticator to get an >> authenticated Subject. None of this requires the SM. > > I see that in `RMIConnectionImpl.java` it is stored inside an > `AccessControlContext`, and there are `doPrivileged` calls on this ACC to > pass the subject into an action. So, even if there might be no SM but the > subject will never be bound to a thread using a scoped value. > > I’ll revert the change then, and this code must have SM allowed to run > correctly. If user runs it with SM disabled, at least they will see an UOE > instead of letting `current()` silently returns a null. > > Ultimately, if we want it working after SM, we should update > `RMIConnectionImpl` and rewrite the ACC-related code to using > `Subject.callAs`. Yes - the JMX implementation stores and retrieve subjects in the AccessControlContext and then uses doPrivileged. Subject.doAs is not used in the JMX implementation. There are two different uses of Subject in JMX: 1. one is a simplified role-based authentication/authorization at the default agent level. 2. The other one is a permission check where different subjects can be granted different privileges in the policy file. The latter will go away since the SM is going away, but needs to be preserved until then. The former we want to keep and needs to keep working (by changing the code to use callAs) even after the SM is gone. Subject delegation allows an authenticated subject (1. above) to perform actions on behalf of a delegation subject, where the privileged granted by 2. are the intersection of the privileges of the two subjects. @kevinjwalls is currently working on removing subject delegation. - PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1515896397
Re: RFR: 8326687: Inconsistent use of "ZIP", "Zip" and "zip" in java.util.zip/jar zipfs javadoc
On Mon, 26 Feb 2024 19:55:47 GMT, Lance Andersen wrote: > This PR updates the javadoc and comments within java.util.zip/jar and zipfs > module summary so that it is consistent with the use of "ZIP". > > In addition, open/src/java.base/share/classes/java/util/zip/package-info.java > has been updated to point to the higher level location of the PKWARE > APPNOTE.TXT has PKWare recently changed the direct path the the latest > version of the spec. > > It is also worth noting that error messages were not updated as part of the > PR and will be updated separately to keep the javadoc changes separate I am not a usual Reviewer for this area but the changes LGTM Lance. I haven't seen anything unexpected given the description of the changes. - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18011#pullrequestreview-1903392981
Re: RFR: 8320699: Add parameter to skip progress logging of OutputAnalyzer [v4]
On Mon, 15 Jan 2024 08:36:43 GMT, Stefan Karlsson wrote: >> Tests using ProcessTools.executeProcess gets the following output written to >> stdout: >> [2023-11-24T09:58:16.797540608Z] Gathering output for process 2517117 >> [2023-11-24T09:58:23.070781345Z] Waiting for completion for process 2517117 >> [2023-11-24T09:58:23.071045055Z] Waiting for completion finished for process >> 2517117 >> >> This might be good for some use cases and debugging, but some tests spawns a >> large number of processes and for those this output fills up the log files. >> >> I propose that we add a way to turn of this output for tests where we find >> this output to be too noisy. > > Stefan Karlsson has updated the pull request incrementally with one > additional commit since the last revision: > > Update copyright years Hi Stefan - I see that this is an opt-in, so LGTM - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16807#pullrequestreview-1901818783
Re: RFR: 8310994: Add JFR event for selection operations [v3]
On Wed, 3 Jan 2024 11:11:40 GMT, Alan Bateman wrote:
>> Tim Prinzing has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> add select timeout field to the event
>
> src/java.base/share/classes/sun/nio/ch/SelectorImpl.java line 153:
>
>> 151: if ((n == 0) || (SelectorSelectEvent.shouldCommit(duration))) {
>> 152: timeout = (timeout < 0) ? 0 : timeout;
>> 153: SelectorSelectEvent.commit(start, duration, n, timeout);
>
> The comment is a bit confusing here. n == 0 means that no selected keys were
> added or consumed before timeout or wakeup.
Is n == 0 intended to detect a spinning condition where the selector goes back
into select when the event has not been handled?
In that case should we still emit an event if a timeout is present and the
duration is greater than the timeout? For instance, in the HttpClient, we have
a selector thread that will wake up at regular interval - we set up a timeout
which is the min between the next expected timer event and 1500ms. So that
could potentially fire an event every 1500ms if for instance the connection
threshold is less than 1500ms and the connection is idle.
Maybe that's OK - and maybe in that case the onus is on the user to set a
threshold greater than 1500ms?
Or should it be ((n == 0 && durationToMillis(duration) <
timeoutToMillis(timeout)) || ...)) (duration and timeout probably are not in
the same unit of time) - also if shouldCommit return false will the event
actually be emitted down the road? Because if not then the ( n== 0 || ...)
won't work.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1502681499
Re: RFR: 8310994: Add JFR event for selection operations [v2]
On Wed, 13 Dec 2023 18:49:08 GMT, Daniel Fuchs wrote:
>> Tim Prinzing 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 12 additional
>> commits since the last revision:
>>
>> - Change event generation:
>>
>>- selectNow is filtered out
>>- select that times out is always sent
>>- select without timeout uses duration test
>> - rename event to SelectorSelect, field to selectionKeyCount.
>> - Merge branch 'master' into JDK-8310994
>> - remove trailing whitespace
>> - event logic outside of the lock, selector in try block
>> - remove unused import
>> - fix TestConfigure failure
>> - add event defaults
>> - Merge branch 'master' into JDK-8310994
>> - minor test cleanup
>> - ... and 2 more: https://git.openjdk.org/jdk/compare/9896b3fb...2f7dafd8
>
> src/jdk.jfr/share/classes/jdk/jfr/events/SelectorSelectEvent.java line 44:
>
>> 42: @Label("SelectionKey Count")
>> 43: @Description("Number of channels ready for I/O or added to ready
>> set")
>> 44: public int selectionKeyCount;
>
> same here
Thanks for adding the timeout.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1502686107
Re: RFR: 8310994: Add JFR event for selection operations [v2]
On Wed, 13 Dec 2023 18:58:40 GMT, Tim Prinzing wrote:
>> src/java.base/share/classes/jdk/internal/event/SelectorSelectEvent.java line
>> 41:
>>
>>> 39: public class SelectorSelectEvent extends Event {
>>> 40:
>>> 41: public int selectionKeyCount;
>>
>> I still believe we should record the timeout parameter in the event.
>
> Good point about the wakeup. I'll add the field.
Thanks!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1502695847
Re: CFV: New Core Libraries Group Member: Viktor Klang
Vote: yes best regards, -- daniel On 19/02/2024 23:40, Stuart Marks wrote: I hereby nominate Viktor Klang [6] to Membership in the Core Libraries Group [4].
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v5]
On Fri, 16 Feb 2024 10:27:11 GMT, Christoph Langer wrote: >> During analysing a customer case I figured out that we have an inconsistency >> between documentation and actual behavior in class >> com.sun.jndi.ldap.Connection. The [method documentation of >> com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) >> states: "If a timeout is supplied but unconnected sockets are not supported >> then the timeout is ignored and a connected socket is created." >> >> This, however does not happen. If a SocketFactory would not support >> unconnected sockets, it would likely throw a SocketException in >> [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). >> And since [the >> code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) >> does not check for this behavior, a connection with timeout value through a >> SocketFactory that does not support unconnected sockets would simply fail >> with an IOException. >> >> So we should either make the code adhere to what is documented or adapt the >> documentation to the actual behavior. >> >> I hereby try to fix the connect coding. Alternatively, we could also adapt >> the description - I have no strong opinion. What do the experts suggest? > > Christoph Langer 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 five additional > commits since the last revision: > > - Typo > - Merge branch 'master' into JDK-8325579 > - Rename test and refine comment > - Enhance test > - JDK-8325579 test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeTest.java line 62: > 60: * whether the socket is closed after closing the Context. > 61: * > 62: * @run main/othervm --add-opens java.naming/javax.naming=ALL-UNNAMED > --add-opens java.naming/com.sun.jndi.ldap=ALL-UNNAMED sigh... git handles the renaming of this test file as a deleted file + a new file which makes it hard to review the changes. The --add-opens directive are very strange here. I suggest removing them and replacing them with a single `@modules` directive: @modules java.naming/javax.naming:+open java.naming/com.sun.jndi.ldap:+open - PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1492725787
Re: CFV: New Core Libraries Group Member: Raffaello Giulietti
Vote: yes best regards, -- daniel On 13/02/2024 20:25, Brian Burkhalter wrote: I hereby nominate Raffaello Giulietti to Membership in the Core Libraries Group.
Re: RFR: 8325558: Add jcheck whitespace checking for properties files
On Fri, 9 Feb 2024 13:35:55 GMT, Magnus Ihse Bursie wrote: > This is an attempt to finally implement the idea brought forward in > JDK-8295729: Properties files is essentially source code. It should have the > same whitespace checks as all other source code, so we don't get spurious > trailing whitespace changes or leading tabs instead of spaces. > > With Skara jcheck, it is possible to increase the coverage of the whitespace > checks. > > However, this turned out to be problematic, since trailing whitespace is > significant in properties files. That issue has mostly been sorted out in a > series of PRs, and this patch will finish the job with the few remaining > files, and actually enable the check in jcheck. Approving the sun.net changes. - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17789#pullrequestreview-1875818676
Re: RFR: 8325558: Add jcheck whitespace checking for properties files
On Fri, 9 Feb 2024 13:35:55 GMT, Magnus Ihse Bursie wrote: > This is an attempt to finally implement the idea brought forward in > JDK-8295729: Properties files is essentially source code. It should have the > same whitespace checks as all other source code, so we don't get spurious > trailing whitespace changes or leading tabs instead of spaces. > > With Skara jcheck, it is possible to increase the coverage of the whitespace > checks. > > However, this turned out to be problematic, since trailing whitespace is > significant in properties files. That issue has mostly been sorted out in a > series of PRs, and this patch will finish the job with the few remaining > files, and actually enable the check in jcheck. changes to sun/net content-types.properties look OK - PR Comment: https://git.openjdk.org/jdk/pull/17789#issuecomment-1935996382
Re: RFR: JDK-8325189: Enable this-escape javac warning in java.base
On Tue, 6 Feb 2024 17:29:25 GMT, Joe Darcy wrote:
>> src/java.base/share/classes/sun/net/www/MessageHeader.java line 53:
>>
>>> 51: }
>>> 52:
>>> 53: @SuppressWarnings("this-escape")
>>
>> An alternative here could be to make the class final. AFAICS it's not
>> subclassed anywhere. If you'd prefer not to do this here then maybe a
>> followup issue could be logged?
>
> I'd prefer if that kind of change were done as a subtask of
>
> [JDK-8325263](https://bugs.openjdk.org/browse/JDK-8325263): Address
> this-escape lint warnings java.base (umbrella)
Thanks Joe. I logged
[JDK-8325361](https://bugs.openjdk.org/browse/JDK-8325361): Make
sun.net.www.MessageHeader final
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17692#discussion_r1480362736
Re: RFR: JDK-8325189: Enable this-escape javac warning in java.base
On Fri, 2 Feb 2024 23:36:41 GMT, Joe Darcy wrote:
> After the "this-escape" lint warning was added to javac (JDK-8015831), the
> base module was not updated to be able to compile with this warning enabled.
> This PR makes the necessary changes to allow the base module to build with
> the warning enabled.
I looked at the modifications in java.net / sun.net. Looks generally good
though I have some comments.
src/java.base/share/classes/java/net/Socket.java line 323:
> 321: * @seeSecurityManager#checkConnect
> 322: */
> 323: @SuppressWarnings("this-escape")
This is a weird one. I guess the issue here is that the escape happens in the
chained constructor, and is propagated recursively up the constructor chain. Is
the suppress warning here still needed after disabling this-escape warning at
line 358?
Actually - these are all weird since the only place where the escape happens is
in the private constructor at line 548 - and it doesn't even get flagged there
(presumably because it's a private constructor?)
I guess that the rationale is that subclasses cannot override the private
constructor (where the escape happen), but can override the public constructor
that calls the private constructor where the escape happen. I can't help
feeling that the warning would be better placed on the private constructor
though. Seeing it here confused me a lot.
src/java.base/share/classes/sun/net/www/MessageHeader.java line 53:
> 51: }
> 52:
> 53: @SuppressWarnings("this-escape")
An alternative here could be to make the class final. AFAICS it's not
subclassed anywhere. If you'd prefer not to do this here then maybe a followup
issue could be logged?
-
PR Review: https://git.openjdk.org/jdk/pull/17692#pullrequestreview-1865378355
PR Review Comment: https://git.openjdk.org/jdk/pull/17692#discussion_r1479922189
PR Review Comment: https://git.openjdk.org/jdk/pull/17692#discussion_r1479936447
Re: RFR: 8325109: Sort method modifiers in canonical order
On Thu, 1 Feb 2024 11:57:04 GMT, Magnus Ihse Bursie wrote: > This is a follow-up on > [JDK-8324053](https://bugs.openjdk.org/browse/JDK-8324053). I have run the > bin/blessed-modifier-order.sh on the entire code base, and manually checked > the result. I have reverted all but these trivial and uncontroversial changes. > > Almost all of these are about moving `static` to its proper position; a few > do not involve `static` but instead puts `final` or `abstract` in the correct > place. > > I have deliberately stayed away from `default` in this PR, since they should > probably get a more thorough treatment, see > https://github.com/openjdk/jdk/pull/17468#pullrequestreview-1829238473. Changes to IPAdressUtil look fine. I eyeballed the rest and didn't spot any issue. - PR Review: https://git.openjdk.org/jdk/pull/17670#pullrequestreview-1856519410
Re: RFR: 8325001: Typo in the javadocs for the Arena::ofShared method
On Wed, 31 Jan 2024 13:12:10 GMT, Per Minborg wrote: > This PR fixes a typo in the documentation for the `Arena::ofShared`. LGTM - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17653#pullrequestreview-1854679366
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs
On Wed, 17 Jan 2024 23:41:53 GMT, Weijun Wang wrote: > This code change adds an alternative implementation of user-based > authorization `Subject` APIs that doesn't depend on Security Manager APIs. > Depending on if the Security Manager is allowed, the methods store the > current subject differently. See the spec change in the `Subject.java` file > for details. When the Security Manager APIs are finally removed in a future > release, this new implementation will be only implementation for these > methods. > > One major change in the new implementation is that `Subject.getSubject` > always throws an `UnsupportedOperationException` since it has an > `AccessControlContext` argument but the current subject is no longer > associated with an `AccessControlContext` object. > > Now it's the time to migrate from the `getSubject` and `doAs` methods to > `current` and `callAs`. If the user application is simply calling > `getSubject(AccessController.getContext())`, then switching to `current()` > would work. If the `AccessControlContext` argument is retrieved from an > earlier `getContext()` call and the associated subject might be different > from that of the current `AccessControlContext`, then instead of storing the > previous `AccessControlContext` object and passing it into `getSubject` to > get the "previous" subject, the application should store the `current()` > return value directly. Changes requested by dfuchs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17472#pullrequestreview-1851409950
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs
On Tue, 30 Jan 2024 13:53:37 GMT, Daniel Fuchs wrote:
>> This code change adds an alternative implementation of user-based
>> authorization `Subject` APIs that doesn't depend on Security Manager APIs.
>> Depending on if the Security Manager is allowed, the methods store the
>> current subject differently. See the spec change in the `Subject.java` file
>> for details. When the Security Manager APIs are finally removed in a future
>> release, this new implementation will be only implementation for these
>> methods.
>>
>> One major change in the new implementation is that `Subject.getSubject`
>> always throws an `UnsupportedOperationException` since it has an
>> `AccessControlContext` argument but the current subject is no longer
>> associated with an `AccessControlContext` object.
>>
>> Now it's the time to migrate from the `getSubject` and `doAs` methods to
>> `current` and `callAs`. If the user application is simply calling
>> `getSubject(AccessController.getContext())`, then switching to `current()`
>> would work. If the `AccessControlContext` argument is retrieved from an
>> earlier `getContext()` call and the associated subject might be different
>> from that of the current `AccessControlContext`, then instead of storing the
>> previous `AccessControlContext` object and passing it into `getSubject` to
>> get the "previous" subject, the application should store the `current()`
>> return value directly.
>
> src/java.management/share/classes/com/sun/jmx/remote/internal/ServerNotifForwarder.java
> line 349:
>
>> 347: @SuppressWarnings("removal")
>> 348: private Subject getSubject() {
>> 349: return Subject.current();
>
> Since `Subject::current` is not deprecated the annotation at line 347 above
> should be removed.
OK - things seem to be a bit convoluted here and some pieces might be missing.
I suspect that what needs to be done is more complicated:
`RMIConnectionImpl` sets up an ACC and calls doPrivileged with that ACC, on the
assumption that the subject is tied to the ACC and it can be retrieved down the
road from the ACC. `RMIConnectionImpl` has the subject, and the delegation
subject too.
So for `Subject::current` to work here, shouldn't
`RMIConnectionImpl::doPrivilegedOperation` use `Subject::callAs` when the
security manager is disallowed?
It seems that when `Subject::current` is used, some analysis should be done to
verify where the Subject is supposed to come from - that is - how the caller is
expecting the subject to reach the callee.
Typically, JMX doesn't use `Subject::doAs` but ties a `Subject` to an
`AccessControlContext` and uses `doPrivileged` instead.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1471308151
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs
On Wed, 17 Jan 2024 23:41:53 GMT, Weijun Wang wrote:
> This code change adds an alternative implementation of user-based
> authorization `Subject` APIs that doesn't depend on Security Manager APIs.
> Depending on if the Security Manager is allowed, the methods store the
> current subject differently. See the spec change in the `Subject.java` file
> for details. When the Security Manager APIs are finally removed in a future
> release, this new implementation will be only implementation for these
> methods.
>
> One major change in the new implementation is that `Subject.getSubject`
> always throws an `UnsupportedOperationException` since it has an
> `AccessControlContext` argument but the current subject is no longer
> associated with an `AccessControlContext` object.
>
> Now it's the time to migrate from the `getSubject` and `doAs` methods to
> `current` and `callAs`. If the user application is simply calling
> `getSubject(AccessController.getContext())`, then switching to `current()`
> would work. If the `AccessControlContext` argument is retrieved from an
> earlier `getContext()` call and the associated subject might be different
> from that of the current `AccessControlContext`, then instead of storing the
> previous `AccessControlContext` object and passing it into `getSubject` to
> get the "previous" subject, the application should store the `current()`
> return value directly.
src/java.management/share/classes/com/sun/jmx/remote/internal/ServerNotifForwarder.java
line 349:
> 347: @SuppressWarnings("removal")
> 348: private Subject getSubject() {
> 349: return Subject.current();
Since `Subject::current` is not deprecated the annotation at line 347 above
should be removed.
src/java.management/share/classes/com/sun/jmx/remote/security/MBeanServerFileAccessController.java
line 307:
> 305: AccessController.doPrivileged(new PrivilegedAction<>() {
> 306: public Subject run() {
> 307: return Subject.current();
Is the `doPrivileged` still needed here? Is there a chance that
`Subject.current()` will throw a `SecurityException`, or return a different
result if a security manager is present and `doPrivileged` is not used?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1471257982
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1471263581
Re: RFR: 8324053: Use the blessed modifier order for sealed in java.base
On Wed, 17 Jan 2024 21:22:07 GMT, Pavel Rappo wrote: > Please review this trivial PR to reorder the `sealed` class or interface > modifier. For context of this change see: > https://github.com/openjdk/jdk/pull/17242#issuecomment-1887338396. LGTM - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17468#pullrequestreview-1829273407
Re: RFR: 8323562: SaslInputStream.read() may return wrong value
On Thu, 11 Jan 2024 06:28:51 GMT, Sergey Bylokhov wrote: > SaslInputStream.read() should return a value in the range from 0 to 255 per > the spec of InputStream.read() but it returns the signed byte from the inBuf > as is. Marked as reviewed by dfuchs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17365#pullrequestreview-1818425451
Re: RFR: 8323562: SaslInputStream.read() may return wrong value
On Fri, 12 Jan 2024 11:54:06 GMT, Alan Bateman wrote: > I think this one will require digging into whether the no-arg read is used in > the authentication or not. It might not be, in which case it's not testable > with something that emulates LDAPv3. However if it is used then we should > have fuzzing or other tests to exercise it. I'm not saying it should be part > of this PR but finding a 15+ year issue in authentication code is concerning > so will need follow-up. AFAICT the no arg read() method is never called by the JNDI/LDAP stack. This explains why it never made any test fail. - PR Comment: https://git.openjdk.org/jdk/pull/17365#issuecomment-1889065309
Re: RFR: 8275338: Add JFR events for notable serialization situations [v12]
On Tue, 9 Jan 2024 10:46:27 GMT, Raffaello Giulietti wrote: >> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java >> line 70: >> >>> 68: privilegedCheckAccessibleMethod(cl, WRITE_REPLACE_NAME, >>> 69: WRITE_REPLACE_PARAM_TYPES, Object.class); >>> 70: privilegedCheckAccessibleMethod(cl, READ_RESOLVE_NAME, >> >> Thinking ahead to when the security manager is removed, can the code that >> needs private access to field values (SUID) be more narrowly accessed? >> Perhaps switch to using a VarHandle and MethodHandles.Lookup. This may be a >> longer term issue and beyond the scope of this PR. >> >> In the naming of the `PrivilegedXXX` methods, I would drop the "privileged" >> part of the name. >> The methods do not change the privilege level and operate correctly if when >> the caller has the privileges needed. And all of these methods are read-only >> so there is no/little risk in their implementations and avoid refitting the >> terminology later. > > They are called `privilegedXXX` because they _are_ (already) privileged, not > because they change the privileges. > > But yes, in view of removing the security manager, the implementation could > be more "modern". Will have a look. > https://github.com/openjdk/jdk/pull/17129/commits/ff1b7068bd902f3030267afbc468e3244c944604 Thanks - that looks much cleaner. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1447617134
Re: RFR: 8275338: Add JFR events for notable serialization situations [v12]
On Tue, 9 Jan 2024 10:42:58 GMT, Raffaello Giulietti
wrote:
>> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
>> line 87:
>>
>>> 85: }
>>> 86: if (cl.isEnum()) {
>>> 87: commitEvent(cl, SUID_NAME + " in an enum class is not
>>> effective");
>>
>> Is there a best practice that can be included in the message? "SUID should
>> not be declared"?
>
> Yes, that's perhaps clearer, will do.
Typically in other places in the JDK we use `priviledgedXxx` for naming methods
that are simple wrappers that call `xxx` from within a `doPrivileged`. The
method is called privileged because it doesn't require the caller to use
`doPrivileged`.
That is something like:
String privilegedGetProperty(String property) {
return AccessController.doPrivileged((...) () ->
System.getProperty(property));
}
See for instance:
https://github.com/openjdk/jdk/blob/ee98d262181f5822609674c71c85ad4576ac1632/src/java.base/share/classes/sun/security/action/GetPropertyAction.java#L107
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1446460008
Re: RFR: 8275338: Add JFR events for notable serialization situations [v3]
On Tue, 19 Dec 2023 19:39:22 GMT, Roger Riggs wrote: >> What about fixed `String`s rather than `int`s for the kind of error? >> Something like `"SUID_INEFFECTIVE_ON_ENUM"`, and so on? >> It would be nice to be able to use enums, but AFAIK that's not supported in >> JFR. > > You could define them with an Enum but use the ordinal as the value for JFR. Same remark here about finality as https://github.com/openjdk/jdk/pull/17129#discussion_r1432400888. public statics should be final. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1432402527
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 16:00:09 GMT, Raffaello Giulietti wrote: >> src/java.base/share/classes/jdk/internal/event/SerializationMisdeclarationEvent.java >> line 94: >> >>> 92: >>> 93: /* >>> 94: * These constants are not final on purpose. >> >> Just curious - what's the purpose? I don't see them being changed/updated >> anywhere (not even in tests). > > Declaring a `public static int` field `final` means that the value is then > stored in the client class. If a value is changed, then the client needs to > be recompiled as well, but this is not enforced by the language, so it might > lead to inconsistencies. > > The clean solution would be to use an enum class, but that's not supported by > JFR. Not having them final is a quite smelly. I would suggest to make them final with a comment that new values can be added but that existing values must not be changed. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1432400888
Re: RFR: 8310994: Add JFR event for selection operations [v2]
On Wed, 13 Dec 2023 18:38:11 GMT, Tim Prinzing wrote: >>> It could also be interesting to provide the `timeout` that was given to the >>> selection operation. >> >> I've tried to work through issues, esp. around selector spinning, and being >> able to distinguish select from selectNow is important for all of them, so >> yes, the timeout is needed or else no emit when the timeout == 0 as that's >> the case you have to filter out when troubleshooting. > > I've added filtering of selectNow(), and an event is emitted if there is a > timeout independent of the threshold. The duration should roughly equal the > timout in that case. I added more test cases to cover those two changes. The select call may also exit early with 0 key selected if it was woken up by a call to Selector::wakeup and this is not necessarily indicative of an issue in the API. We use this facility a lot in the HttpClient as we also use the selector thread as a timer thread. - PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1425760691
Re: RFR: 8310994: Add JFR event for selection operations [v2]
On Wed, 13 Dec 2023 18:38:09 GMT, Tim Prinzing wrote:
>> Added mirror event with static methods: jdk.internal.event.SelectionEvent
>> that provides the duration of select calls and the count of how many keys
>> are available.
>>
>> Emit the event from SelectorImpl::lockAndDoSelect
>>
>> Test at jdk.jfr.event.io.TestSelectionEvents
>
> Tim Prinzing 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 12 additional
> commits since the last revision:
>
> - Change event generation:
>
>- selectNow is filtered out
>- select that times out is always sent
>- select without timeout uses duration test
> - rename event to SelectorSelect, field to selectionKeyCount.
> - Merge branch 'master' into JDK-8310994
> - remove trailing whitespace
> - event logic outside of the lock, selector in try block
> - remove unused import
> - fix TestConfigure failure
> - add event defaults
> - Merge branch 'master' into JDK-8310994
> - minor test cleanup
> - ... and 2 more: https://git.openjdk.org/jdk/compare/ddad6141...2f7dafd8
src/java.base/share/classes/jdk/internal/event/SelectorSelectEvent.java line 41:
> 39: public class SelectorSelectEvent extends Event {
> 40:
> 41: public int selectionKeyCount;
I still believe we should record the timeout parameter in the event.
src/jdk.jfr/share/classes/jdk/jfr/events/SelectorSelectEvent.java line 44:
> 42: @Label("SelectionKey Count")
> 43: @Description("Number of channels ready for I/O or added to ready set")
> 44: public int selectionKeyCount;
same here
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1425754281
PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1425755618
Re: RFR: 8316649: JMX connection timeout cannot be changed and uses the default of 0 (infinite)
On Mon, 27 Nov 2023 15:00:54 GMT, Daniel Fuchs wrote: >> On the Property name: there is an existing System Property >> "sun.rmi.transport.connectionTimeout" which is a 15-second idle timeout. >> Having a "connectionTimeout" and this new one as "connectTimeout" could be >> confusing, even in different but very similar package names, hence naming >> this "initialConnectTimeout". >> (So "initial" to signal that it's the initial connect of a socket, different >> to the existing "connectionTimeout".) >> It does not seem like the kind of system property a user would expect to be >> read again on every usage, I think, let me know if you see that as a problem. >> >> I am hoping that as we already have various properties fetched in the same >> manner, we don't see the need to pursue a test that the value is fetched and >> passed on. > > Have you considered naming it "socketConnectTimeout" instead? Or possibly > "tcpConnectTimeout"? If you named it "tcpConnectTimeout" then you could name the (future) property for [JDK-8320703](https://bugs.openjdk.org/browse/JDK-8320703: JMX SSL RMI Socket connection timeout cannot be changed) "tlsConnectTimeout"... - PR Review Comment: https://git.openjdk.org/jdk/pull/16771#discussion_r1406309820
Re: RFR: 8316649: JMX connection timeout cannot be changed and uses the default of 0 (infinite)
On Mon, 27 Nov 2023 14:35:07 GMT, Kevin Walls wrote: >> wrt to the property name initialConnectTimeout might infer that it is an >> initial value which can be subsequentually modified, but that is not >> possible. As such, would sun.rmi.transport.tcp.connectTimeout be more >> appropriate -- dropping the initial ? >> >> If the connectTimeout initialization code was placed in a static method, it >> could be directly unit testable :-) (if such a test was desirable for >> coverage completeness) > > On the Property name: there is an existing System Property > "sun.rmi.transport.connectionTimeout" which is a 15-second idle timeout. > Having a "connectionTimeout" and this new one as "connectTimeout" could be > confusing, even in different but very similar package names, hence naming > this "initialConnectTimeout". > (So "initial" to signal that it's the initial connect of a socket, different > to the existing "connectionTimeout".) > It does not seem like the kind of system property a user would expect to be > read again on every usage, I think, let me know if you see that as a problem. > > I am hoping that as we already have various properties fetched in the same > manner, we don't see the need to pursue a test that the value is fetched and > passed on. Have you considered naming it "socketConnectTimeout" instead? - PR Review Comment: https://git.openjdk.org/jdk/pull/16771#discussion_r1406293632
Re: RFR: 8316649: JMX connection timeout cannot be changed and uses the default of 0 (infinite)
On Fri, 24 Nov 2023 12:13:56 GMT, Kevin Walls wrote: >> (Look for socket factories in the module `jdk.management.agent`) > > OK yes, we also have: > java.rmi/share/classes/javax/rmi/ssl/SslRMIClientSocketFactory.java > with its own createSocket(String host, int port) method. This is used if we > use JMX over SSL. > > So SslRMIClientSocketFactory could specifically implement the connect timeout. > > Next q, should it? 8-) > > The reported hang and those I have seen in testing have only been in: > sun.rmi.transport.tcp.TCPDirectSocketFactory.createSocket calling Socket.init. > > javax/rmi/ssl/SslRMIClientSocketFactory.java reads some properties named > "javax.rmi.ssl.client" > so it would be odd for it to read > "sun.rmi.transport.tcp.initialConnectTimeout" I was proposing here. > > It could implement "javax.rmi.ssl.client.initialConnectTimeout", or we could > leave SSL alone for now, possibly handling it in a separate issue if it's > wanted. OK - sounds good. Meanwhile I had a look at the custom RMI Socket Factories used by the JMX Agent, and these are actually RMIServerSocketFactories, so having a timeout for connect there probably makes no sense. - PR Review Comment: https://git.openjdk.org/jdk/pull/16771#discussion_r1404331875
Re: RFR: 8316649: JMX connection timeout cannot be changed and uses the default of 0 (infinite)
On Thu, 23 Nov 2023 17:17:34 GMT, Daniel Fuchs wrote: >>> I see Integer.getInteger handles the obvious Exceptions, so specifying a >>> strange value does not immediately break us. >> >> Oh - right. It's `Integer.getInteger`, not `Integer.parseInt` Ok, then - I >> withdraw my first comment :-) > >> This is a stack from a test I was experimenting with, when it did see the >> timeout: > > Ah - that's for testing with a particular test. So the question now is: > > Should the RMISocketFactories provided / implemented in the JMX > implementation - such as those used by the JMX default agent, also honour > this system property? (Look for socket factories in the module `jdk.management.agent`) - PR Review Comment: https://git.openjdk.org/jdk/pull/16771#discussion_r1403622189
Re: RFR: 8316649: JMX connection timeout cannot be changed and uses the default of 0 (infinite)
On Thu, 23 Nov 2023 17:14:52 GMT, Daniel Fuchs wrote: >>> IIRC, the default agent uses / may use its own socket factories - are we >>> still coming here even with those factories? >> >> We do get here, yes (not saying you can't customise your way out of getting >> here). This is a stack from a test I was experimenting with, when it did >> see the timeout: >> >> >> -System.out:(4/132)-- >> JMX URL = service:jmx:rmi://x >> expectTimeout = true >> sun.rmi.transport.tcp.initialConnectTimeout = 1 >> Test passed >> --System.err:(24/1791)-- >> java.rmi.ConnectIOException: Exception creating connection to: x.x.x.x; >> nested exception is: >> java.net.SocketTimeoutException >> at >> java.rmi/sun.rmi.transport.tcp.TCPEndpoint.newSocket(TCPEndpoint.java:637) >> at >> java.rmi/sun.rmi.transport.tcp.TCPChannel.createConnection(TCPChannel.java:217) >> at >> java.rmi/sun.rmi.transport.tcp.TCPChannel.newConnection(TCPChannel.java:204) >> at java.rmi/sun.rmi.server.UnicastRef.invoke(UnicastRef.java:134) >> at >> java.management.rmi/javax.management.remote.rmi.RMIServerImpl_Stub.newClient(RMIServerImpl_Stub.java:85) >> at >> java.management.rmi/javax.management.remote.rmi.RMIConnector.getConnection(RMIConnector.java:2106) >> at >> java.management.rmi/javax.management.remote.rmi.RMIConnector.connect(RMIConnector.java:321) >> at >> java.management/javax.management.remote.JMXConnectorFactory.connect(JMXConnectorFactory.java:270) >> at >> java.management/javax.management.remote.JMXConnectorFactory.connect(JMXConnectorFactory.java:229) >> at RMIConnectionTimeoutTest.main(RMIConnectionTimeoutTest.java:65) >> at >> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103) >> at java.base/java.lang.reflect.Method.invoke(Method.java:580) >> at >> com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138) >> at java.base/java.lang.Thread.run(Thread.java:1570) >> Caused by: java.net.SocketTimeoutException >> at >> java.base/java.net.SocksSocketImpl.remainingMillis(SocksSocketImpl.java:112) >> at >> java.base/java.net.SocksSocketImpl.connect(SocksSocketImpl.java:327) >> at java.base/java.net.Socket.connect(Socket.java:752) >> at >> java.rmi/sun.rmi.transport.tcp.TCPDirectSocketFactory.createSocket(TCPDirectSocketFactory.java:57) >> at >> java.rmi/sun.rmi.transport.tcp.TCPEndpoint.newSocket(TCPEndpoint.java:619) >> ... 13 more >> STATUS:Passed. > >> I see Integer.getInteger handles the obvious Exceptions, so specifying a >> strange value does not immediately break us. > > Oh - right. It's `Integer.getInteger`, not `Integer.parseInt` Ok, then - I > withdraw my first comment :-) > This is a stack from a test I was experimenting with, when it did see the > timeout: Ah - that's for testing with a particular test. So the question now is: Should the RMISocketFactories provided / implemented in the JMX implementation - such as those used by the JMX default agent, also honour this system property? - PR Review Comment: https://git.openjdk.org/jdk/pull/16771#discussion_r1403620057
Re: RFR: 8316649: JMX connection timeout cannot be changed and uses the default of 0 (infinite)
On Thu, 23 Nov 2023 12:43:33 GMT, Kevin Walls wrote: >>> An exception here will prevent the class from being initialized... >> >> Maybe we can break it, but this new property is following the same pattern I >> think as the neighbouring file >> src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java when it >> reads the system props. I see Integer.getInteger handles the obvious >> Exceptions, so specifying a strange value does not immediately break us. >> >> If there's more protection needed then we have various other places to apply >> it that might need separate follow-up if you think there's a case? > >> IIRC, the default agent uses / may use its own socket factories - are we >> still coming here even with those factories? > > We do get here, yes (not saying you can't customise your way out of getting > here). This is a stack from a test I was experimenting with, when it did see > the timeout: > > > -System.out:(4/132)-- > JMX URL = service:jmx:rmi://x > expectTimeout = true > sun.rmi.transport.tcp.initialConnectTimeout = 1 > Test passed > --System.err:(24/1791)-- > java.rmi.ConnectIOException: Exception creating connection to: x.x.x.x; > nested exception is: > java.net.SocketTimeoutException > at > java.rmi/sun.rmi.transport.tcp.TCPEndpoint.newSocket(TCPEndpoint.java:637) > at > java.rmi/sun.rmi.transport.tcp.TCPChannel.createConnection(TCPChannel.java:217) > at > java.rmi/sun.rmi.transport.tcp.TCPChannel.newConnection(TCPChannel.java:204) > at java.rmi/sun.rmi.server.UnicastRef.invoke(UnicastRef.java:134) > at > java.management.rmi/javax.management.remote.rmi.RMIServerImpl_Stub.newClient(RMIServerImpl_Stub.java:85) > at > java.management.rmi/javax.management.remote.rmi.RMIConnector.getConnection(RMIConnector.java:2106) > at > java.management.rmi/javax.management.remote.rmi.RMIConnector.connect(RMIConnector.java:321) > at > java.management/javax.management.remote.JMXConnectorFactory.connect(JMXConnectorFactory.java:270) > at > java.management/javax.management.remote.JMXConnectorFactory.connect(JMXConnectorFactory.java:229) > at RMIConnectionTimeoutTest.main(RMIConnectionTimeoutTest.java:65) > at > java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103) > at java.base/java.lang.reflect.Method.invoke(Method.java:580) > at > com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138) > at java.base/java.lang.Thread.run(Thread.java:1570) > Caused by: java.net.SocketTimeoutException > at > java.base/java.net.SocksSocketImpl.remainingMillis(SocksSocketImpl.java:112) > at > java.base/java.net.SocksSocketImpl.connect(SocksSocketImpl.java:327) > at java.base/java.net.Socket.connect(Socket.java:752) > at > java.rmi/sun.rmi.transport.tcp.TCPDirectSocketFactory.createSocket(TCPDirectSocketFactory.java:57) > at > java.rmi/sun.rmi.transport.tcp.TCPEndpoint.newSocket(TCPEndpoint.java:619) > ... 13 more > STATUS:Passed. > I see Integer.getInteger handles the obvious Exceptions, so specifying a > strange value does not immediately break us. Oh - right. It's `Integer.getInteger`, not `Integer.parseInt` Ok, then - I withdraw my first comment :-) - PR Review Comment: https://git.openjdk.org/jdk/pull/16771#discussion_r1403618437
Re: RFR: 8316649: JMX connection timeout cannot be changed and uses the default of 0 (infinite)
On Tue, 21 Nov 2023 17:57:32 GMT, Kevin Walls wrote:
> RMI Connections (in general) should use a timeout on the Socket connect call
> by default.
>
> JMX connections use RMI and some connection failures never terminate. The
> hang described in 8316649 is hard to reproduce manually: the description says
> it can be caused by a firewall that never returns a response.
>
> src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java
> has other timeouts but nothing to control the initial Socket connect.
>
> Defaulting to a 1 minute timeout on connect has no effect on existing tests
> for RMI and JMX, and should go unnoticed in applications unless there really
> is a significant connection delay. Specifying zero for the new System
> Property sun.rmi.transport.tcp.initialConnectTimeout uses the old code path
> of a connect with no timeout.
>
> I have a test, but it is not realistically usable: although specifying a 1
> millisecond timeout will often fail (as expected/desired for the test), it
> will often pass as the connection happens immediately.
src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPDirectSocketFactory.java
line 46:
> 44: private static final int connectTimeout =// default 1 minute
> 45: AccessController.doPrivileged((PrivilegedAction) () ->
> 46:
> Integer.getInteger("sun.rmi.transport.tcp.initialConnectTimeout", 60 *
> 1000).intValue());
An exception here will prevent the class from being initialized, any subsequent
attempts to use the class will then produce hard to diagnose error. If the
class is not loaded by the main thread, such exception/error could be swallowed
unless an uncaught exception handler was registered. I would suggest
implementing the logic in a static block, catch any exception and revert to
default behaviour (i.e. 0) if the value supplied for the property is not an
integer, or not a valid integer, etc...
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16771#discussion_r1403258620
Re: RFR: 8316649: JMX connection timeout cannot be changed and uses the default of 0 (infinite)
On Thu, 23 Nov 2023 11:35:11 GMT, Daniel Fuchs wrote:
>> RMI Connections (in general) should use a timeout on the Socket connect call
>> by default.
>>
>> JMX connections use RMI and some connection failures never terminate. The
>> hang described in 8316649 is hard to reproduce manually: the description
>> says it can be caused by a firewall that never returns a response.
>>
>> src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java
>> has other timeouts but nothing to control the initial Socket connect.
>>
>> Defaulting to a 1 minute timeout on connect has no effect on existing tests
>> for RMI and JMX, and should go unnoticed in applications unless there really
>> is a significant connection delay. Specifying zero for the new System
>> Property sun.rmi.transport.tcp.initialConnectTimeout uses the old code path
>> of a connect with no timeout.
>>
>> I have a test, but it is not realistically usable: although specifying a 1
>> millisecond timeout will often fail (as expected/desired for the test), it
>> will often pass as the connection happens immediately.
>
> src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPDirectSocketFactory.java
> line 46:
>
>> 44: private static final int connectTimeout =// default 1 minute
>> 45: AccessController.doPrivileged((PrivilegedAction) () ->
>> 46:
>> Integer.getInteger("sun.rmi.transport.tcp.initialConnectTimeout", 60 *
>> 1000).intValue());
>
> An exception here will prevent the class from being initialized, any
> subsequent attempts to use the class will then produce hard to diagnose
> error. If the class is not loaded by the main thread, such exception/error
> could be swallowed unless an uncaught exception handler was registered. I
> would suggest implementing the logic in a static block, catch any exception
> and revert to default behaviour (i.e. 0) if the value supplied for the
> property is not an integer, or not a valid integer, etc...
IIRC, the default agent uses / may use its own socket factories - are we still
coming here even with those factories?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16771#discussion_r1403260460
Re: RFR: 8310994: Add JFR event for selection operations
On Fri, 17 Nov 2023 16:22:55 GMT, Tim Prinzing wrote:
> Added mirror event with static methods: jdk.internal.event.SelectionEvent
> that provides the duration of select calls and the count of how many keys are
> available.
>
> Emit the event from SelectorImpl::lockAndDoSelect
>
> Test at jdk.jfr.event.io.TestSelectionEvents
src/java.base/share/classes/jdk/internal/event/SelectionEvent.java line 35:
> 33: * {@link #commit(long, long, int)} method
> 34: * must be the same as the order of the fields.
> 35: */
You should probably define what a "selection operation" is and put a link to
`Selector::select`.
src/java.base/share/classes/jdk/internal/event/SelectionEvent.java line 38:
> 36: public class SelectionEvent extends Event {
> 37:
> 38: public int count;
It could also be interesting to provide the `timeout` that was given to the
selection operation.
src/java.base/share/classes/sun/nio/ch/SelectorImpl.java line 150:
> 148: long duration = SelectionEvent.timestamp() - start;
> 149: if (SelectionEvent.shouldCommit(duration)) {
> 150: SelectionEvent.commit(start, duration, n);
Maybe the value of `timeout` should be provided here?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1401948063
PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1401950078
PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1401951825
Re: RFR: JDK-8319569: Several java/util tests should be updated to accept VM flags [v2]
On Fri, 17 Nov 2023 20:20:43 GMT, Justin Lu wrote: >> Please review this PR which allows these _j.util_ tests to launch new JVM >> processes with VM flags, >> >> This is primarily done using by switching to >> `ProcessTools::createTestJavaProcessBuilder`. >> >> _PropertiesTest.sh_ was updated with `@requires vm.flagless`. > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > reflect review comments Changes to the logging tests LGTM. Thanks for the update @justin-curtis-lu . - PR Comment: https://git.openjdk.org/jdk/pull/16705#issuecomment-1819284286
Re: RFR: 8317834: java/lang/Thread/IsAlive.java timed out
On Tue, 14 Nov 2023 16:41:31 GMT, Darragh Clarke wrote: > Currently the `IsAlive` test occasionally times out. > > This PR changes the timeout from `10` to `25` which I think is an adequate > increase based on the failures I've seen. Though I'd be happy to change it to > another value if someone thinks `25` isn't ideal. Looks reasonable to me. Let's give it at least 48 hours before integrating though, to let folks who might have an opinion (@dholmes-ora ?) to chime in. - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16659#pullrequestreview-1730279358
Re: RFR: JDK-8319123 : Implementation of JEP-461: Stream Gatherers (Preview) [v3]
On Fri, 10 Nov 2023 09:32:17 GMT, Viktor Klang wrote:
>> This Pull-Request implements [JEP-461](https://openjdk.org/jeps/461)
>
> Viktor Klang has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Multiple improvements to Gatherer and Gatherers javadoc and restructuring
> of Gatherers.java to put public at the top of the file.
> - Augmenting Gatherer tests to include default implementation in Stream
Very nice API. Some minor suggestions and comments (and one fix for one of the
examples). I only looked at the API.
src/java.base/share/classes/java/util/stream/Gatherer.java line 45:
> 43: * or be parallelized -- if a combiner function is supplied.
> 44: *
> 45: * Examples of gathering operations include, but is not limited to:
> Examples of ... but _are_ not limited to
src/java.base/share/classes/java/util/stream/Gatherer.java line 115:
> 113: * Gatherer toString = map(i -> i.toString());
> 114: *
> 115: * Gatherer incrementThenToString =
> plusOne.andThen(intToString);
Suggestion:
* Gatherer incrementThenToString =
increment.andThen(intToString);
src/java.base/share/classes/java/util/stream/Gatherer.java line 138:
> 136: * );
> 137: * }
> 138: * }
I would have appreciated a usage example here - with a stream of input
elements and what such a gatherer would produce as output.
src/java.base/share/classes/java/util/stream/Gatherer.java line 250:
> 248: * @param that the other gatherer
> 249: * @param The type of output of that Gatherer
> 250: * @throws NullPointerException if the argument is null
Suggestion:
* @throws NullPointerException if the argument is {@code null}
src/java.base/share/classes/java/util/stream/Gatherer.java line 312:
> 310: * @param the type of input elements for the new gatherer
> 311: * @param the type of results for the new gatherer
> 312: * @throws NullPointerException if the argument is null
Suggestion:
* @throws NullPointerException if the argument is {@code null}
src/java.base/share/classes/java/util/stream/Gatherer.java line 333:
> 331: * @param the type of input elements for the new gatherer
> 332: * @param the type of results for the new gatherer
> 333: * @throws NullPointerException if any argument is null
Suggestion:
* @throws NullPointerException if any argument is {@code null}
src/java.base/share/classes/java/util/stream/Gatherer.java line 356:
> 354: * @param the type of initializer for the new gatherer
> 355: * @param the type of results for the new gatherer
> 356: * @throws NullPointerException if any argument is null
Suggestion:
* @throws NullPointerException if any argument is {@code null}
src/java.base/share/classes/java/util/stream/Gatherer.java line 402:
> 400: * @param the type of input elements for the new gatherer
> 401: * @param the type of results for the new gatherer
> 402: * @throws NullPointerException if any argument is null
Suggestion:
* @throws NullPointerException if any argument is {@code null}
src/java.base/share/classes/java/util/stream/Gatherer.java line 422:
> 420: * @param the type of input elements for the new gatherer
> 421: * @param the type of results for the new gatherer
> 422: * @throws NullPointerException if any argument is null
Suggestion:
* @throws NullPointerException if any argument is {@code null}
src/java.base/share/classes/java/util/stream/Gatherer.java line 448:
> 446: * @param the type of initializer for the new gatherer
> 447: * @param the type of results for the new gatherer
> 448: * @throws NullPointerException if any argument is null
Suggestion:
* @throws NullPointerException if any argument is {@code null}
src/java.base/share/classes/java/util/stream/Gatherer.java line 492:
> 490: *
> 491: * @apiNote This is best-effort only, once this returns true it
> should
> 492: * never return false again for the same instance.
Suggestion:
* @apiNote This is best-effort only, once this returns {@code true} it
should
* never return {@code false} again for the same instance.
src/java.base/share/classes/java/util/stream/Gatherer.java line 577:
> 575: interface Greedy extends Integrator { }
> 576: }
> 577: }
Looks like there is no newline at the end of this file. Though newline at end
of file is not mandatory it avoids getting warning in some tools (such as IDE
etc...)
Suggestion:
}
src/java.base/share/classes/java/util/stream/Gatherers.java line 80:
> 78: * and the contents of the windows it produces
> 79: * @return a new gatherer which groups elements into fixed-size
> windows
> 80: * @throws IllegalArgumentException when windowSize is less than 1
Suggestion:
* @throws IllegalArgumentException when {@code windowSize} is less than 1
Re: RFR: JDK-8319123 : Implementation of JEP-461: Stream Gatherers (Preview) [v2]
On Thu, 9 Nov 2023 09:34:26 GMT, Viktor Klang wrote: >> I think it's still better to specify this for every method. Many developers >> read the documentation only for the specific method they are going to call, >> using IDE features like quick documentation. > > Yeah, I agree with @amaembo, I think it is important to keep the contract > close to the caller. I don't mind either way - just wanted to note that the notion of blanket statements for `NullPointerException` is used in several areas ([java.util.logging](https://docs.oracle.com/en/java/javase/21/docs/api/java.logging/java/util/logging/package-summary.html), [java.net.http](https://docs.oracle.com/en/java/javase/21/docs/api/java.net.http/java/net/http/package-summary.html) ...). So there is an established precedent if you were inclined to go this route. - PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1387889210
Re: RFR: 8319374: JFR: Remove instrumentation for exception events [v3]
On Wed, 8 Nov 2023 05:34:47 GMT, Erik Gahlin wrote:
>> src/java.base/share/classes/jdk/internal/event/ThrowableTracer.java line 62:
>>
>>> 60: if (ExceptionThrownEvent.enabled()) {
>>> 61: long timestamp = ExceptionThrownEvent.timestamp();
>>> 62: ExceptionThrownEvent.commit(timestamp, message, clazz);
>>
>> Just a drive-by observation, but now you get the timestamp from the event it
>> seems strange to pull it out and then pass it back to commit, instead of
>> having a version of commit that automatically uses the internal timestamp.
>
> I agree, and I have looked into it, but I think it's better to do that
> refactorization separately as it will impact other events.
Just for my own understanding: in this particular case the time stamp is
meaningless because the duration is expected to be 0 (or close to it) since
nothing happens between the time the timestamp is taken and the time the event
is committed. Is that correct?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16493#discussion_r1386635624
Re: RFR: 8316144: Remove unused field jdk.internal.util.xml.impl.XMLStreamWriterImpl.Element._Depth
On Wed, 27 Sep 2023 09:22:35 GMT, Andrey Turbanov wrote: > BTW, Who is Joe? I believe that would be @JoeWang-Java - PR Comment: https://git.openjdk.org/jdk/pull/15692#issuecomment-1741186962
Re: RFR: 8317141: Remove unused validIndex method from URLClassPath$JarLoader
On Fri, 29 Sep 2023 05:41:11 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which removes unused (internal) > method from the `private` `URLClassPath$JarLoader`? > > The `validIndex` method which is being removed here was being used when JAR > index was supported. We removed support for JAR index in > https://bugs.openjdk.org/browse/JDK-8302819. This method is now a leftover > and no longer used. > > The commit in this PR also removes a couple of other member fields in > `URLClassPath$JarLoader`. These fields too were used previously when JAR > index was in use and are no longer used. > > tier1, tier2 and tier3 tests continue to pass with this change. Looks reasonable to me. - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15976#pullrequestreview-1650562494
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v6]
On Tue, 19 Sep 2023 20:51:41 GMT, Tim Prinzing wrote: > The existing JFR tests TestSocketChannelEvents and TestSocketEvents in > jdk.jfr.event.io verify the events are still emitted as expected. Thanks Tim. Should 8308995 be listed in the `@bug` clause of these two tests? - PR Comment: https://git.openjdk.org/jdk/pull/14342#issuecomment-1727528861
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v3]
On Thu, 7 Sep 2023 21:54:44 GMT, Tim Prinzing wrote: > No. I think it's a relic from the distant past though. I think the timeout > field should be removed. It's not used on SocketChannel at all, and it > doesn't seem useful on Socket. Should we log an RFE to that effect? - PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1330294609
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v5]
On Thu, 7 Sep 2023 21:50:17 GMT, Tim Prinzing wrote: >> The socket read/write JFR events currently use instrumentation of java.base >> code using templates in the jdk.jfr modules. This results in some java.base >> code residing in the jdk.jfr module which is undesirable. >> >> JDK19 added static support for event classes. The old instrumentor classes >> should be replaced with mirror events using the static support. >> >> In the java.base module: >> Added two new events, jdk.internal.event.SocketReadEvent and >> jdk.internal.event.SocketWriteEvent. >> java.net.Socket and sun.nio.ch.SocketChannelImpl were changed to make use of >> the new events. >> >> In the jdk.jfr module: >> jdk.jfr.events.SocketReadEvent and jdk.jfr.events.SocketWriteEvent were >> changed to be mirror events. >> In the package jdk.jfr.internal.instrument, the classes >> SocketChannelImplInstrumentor, SocketInputStreamInstrumentor, and >> SocketOutputStreamInstrumentor were removed. The JDKEvents class was updated >> to reflect all of those changes. >> >> The existing tests in test/jdk/jdk/jfr/event/io continue to pass with the >> new implementation: >> Passed: jdk/jfr/event/io/TestSocketChannelEvents.java >> Passed: jdk/jfr/event/io/TestSocketEvents.java >> >> I added a micro benchmark which measures the overhead of handling the jfr >> socket events. >> test/micro/org/openjdk/bench/java/net/SocketEventOverhead.java. >> It needs access the jdk.internal.event package, which is done at runtime >> with annotations that add the extra arguments. >> At compile time the build arguments had to be augmented in >> make/test/BuildMicrobenchmark.gmk > > Tim Prinzing has updated the pull request incrementally with one additional > commit since the last revision: > > More changes from review: > > I didn't like the name of the helper method 'checkForCommit' because it > doesn't indicate that it might commit the event. I also don't like > 'commitEvent' because it might not. Since JFR events are sort of like a > queue I went with a name from collections and called it 'offer' so using > it is something like 'SocketReadEvent.offer(...)' which seems like it > gets the idea across better. Also improved the javadoc for it. > > Removed the comments about being instrumented by JFR in > Socket.SocketInputStream and Socket.SocketOutputStream. > > I went ahead and moved the event commiting out of the finally block so > that we don't emit events when the read/write did not actually happen. > The bugid JDK-8310979 will be used to determine if more should be done > in this area. > > The implRead and implWrite were moved up with the other support methods > for read/write. LGTM. Are there existing that will help verify that the read events and write events are still emitted as expected? If not shouldn't we write some? - PR Review: https://git.openjdk.org/jdk/pull/14342#pullrequestreview-1633551891
Re: RFR: 8316087: Test SignedLoggerFinderTest.java is still failing
On Wed, 13 Sep 2023 11:26:30 GMT, Sean Coffey wrote: > Some log messages from the test may be dropped if the bootstraplogger is in > use at time of log call. (bootstap logger logs at INFO level, the security > event logger logs at DEBUG level) > > Modify the test to use a patched EventHelper class to let it log at INFO > level also, ensuring the bootstrap logger handles such logs. > > I'll log a follow on enhancement where the default logging level for > bootstrap logger can be modified (perhaps via the existing > `jdk.system.logger.level` system property) Looks fine to me as a shorter term solution until the enhancement is done. An alternative enhancement could be to have another system property to configure at which level the EventHelper should log. - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15711#pullrequestreview-1624746847
Re: RFR: 8277954: Replace use of monitors with explicit locks in the JDK LDAP provider implementation
On Thu, 31 Aug 2023 22:48:30 GMT, Aleksei Efimov wrote: > The change proposed in this PR improves the behavior of the JDK JNDI/LDAP > provider when running in a virtual thread. Currently, when an LDAP operation > is performed from a virtual thread context a pinned carrier thread is > detected: > > Thread[#29,ForkJoinPool-1-worker-1,5,CarrierThreads] > java.naming/com.sun.jndi.ldap.Connection.read > reply(Connection.java:444) <== monitors:1 > > java.naming/com.sun.jndi.ldap.LdapClient.ldapBind(LdapClient.java:369) <== > monitors:1 > > java.naming/com.sun.jndi.ldap.LdapClient.authenticate(LdapClient.java:196) > <== monitors:1 > java.naming/com.sun.jndi.ldap.LdapCtx.connect(LdapCtx.java:2896) <== > monitors:1 > > To fix that monitor usages are replaced with `j.u.c` locks. All synchronized > blocks/methods have been replaced with locks even if it is only guarding > memory access - the motivation behind such a decision was to avoid an > analysis of scenarios where a mix of monitors and `j.u.c` locks is used. > > There are three types of mechanical changes done in this PR: > > 1. Classes with `synchronized` blocks or `synchronized` methods have been > updated to include a new `ReentrantLock` field. These new fields are used to > replace `synchronized` blocks/methods. > 1. classes that use notify/wait on object monitor have been updated to use > `ReentrantLock.Condition`s signal/await. > 1. if one class `synchronized` on an instance of another class - the > `ReentrantLock` added in item (1) was made a package-protected to give access > to another class. > > With the proposed changes pinned carrier threads are no longer detected > during execution of LDAP operations. > > Testing: `jdk-tier1` to `jdk-tier3`, other `jndi/ldap` regression and JCK > naming tests show no failures. Look reasonable to me. If tier2 and all jndi tests are still passing, I'm good with it. - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15526#pullrequestreview-1617680805
Re: RFR: 8315696: SignedLoggerFinderTest.java test failed
On Tue, 5 Sep 2023 20:06:41 GMT, Sean Coffey wrote: > Update the recently added LoggerFinder tests to cater for a possible > condition where the test finishes before the boot logger executor service has > flushed its pending data. > > By simulating a slow thread in the ExecutorService used in BootstrapLogger, I > was able to reproduce the issue described. Adding the > `BootstrapLoggerUtils.awaitPending();` call allows the tests to pass. LGTM! Thanks for the update Seán. - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15571#pullrequestreview-1617603386
Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v12]
On Thu, 7 Sep 2023 19:27:14 GMT, Mandy Chung wrote: >> 8268829: Provide an optimized way to walk the stack with Class object only >> >> `StackWalker::walk` creates one `StackFrame` per frame and the current >> implementation >> allocates one `StackFrameInfo` and one `MemberName` objects per frame. Some >> frameworks >> like logging may only interest in the Class object but not the method name >> nor the BCI, >> for example, filters out its implementation classes to find the caller >> class. It's >> similar to `StackWalker::getCallerClass` but allows a predicate to filter >> out the element. >> >> This PR proposes to add `Option::DROP_METHOD_INFO` enum that requests to >> drop the method information. If no method information is needed, a >> `StackWalker` with `DROP_METHOD_INFO` >> can be used instead and such stack walker will save the overhead of >> extracting the method information >> and the memory used for the stack walking. >> >> New factory methods to take a parameter to specify the kind of stack walker >> to be created are defined. >> This provides a simple way for existing code, for example logging >> frameworks, to take advantage of >> this enhancement with the least change as it can keep the existing function >> for traversing >> `StackFrame`s. >> >> For example: to find the first caller filtering a known list of >> implementation class, >> existing code can create a stack walker instance with `DROP_METHOD_INFO` >> option: >> >> >> StackWalker walker = StackWalker.getInstance(Option.DROP_METHOD_INFO, >> Option.RETAIN_CLASS_REFERENCE); >> Optional> callerClass = walker.walk(s -> >> s.map(StackFrame::getDeclaringClass) >> .filter(Predicate.not(implClasses::contains)) >> .findFirst()); >> >> >> If method information is accessed on the `StackFrame`s produced by this >> stack walker such as >> `StackFrame::getMethodName`, then `UnsupportedOperationException` will be >> thrown. >> >> Javadoc & specdiff >> >> https://cr.openjdk.org/~mchung/api/java.base/java/lang/StackWalker.html >> https://cr.openjdk.org/~mchung/jdk22/specdiff/overview-summary.html >> >> Alternatives Considered >> One alternative is to provide a new API: >> ` T walkClass(Function, ? extends T> function)` >> >> In this case, the caller would need to pass a function that takes a stream >> of `Class` object instead of `StackFrame`. Existing code would have to >> modify calls to the `walk` method to `walkClass` and the function body. >> >> ### Implementation Details >> >> A `StackWalker` configured with `DROP_METHOD_INFO` ... > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > Fix @Param due to the rename from default to class+method Marked as reviewed by dfuchs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15370#pullrequestreview-1616112038
Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v11]
On Thu, 7 Sep 2023 18:22:40 GMT, Mandy Chung wrote:
>> 8268829: Provide an optimized way to walk the stack with Class object only
>>
>> `StackWalker::walk` creates one `StackFrame` per frame and the current
>> implementation
>> allocates one `StackFrameInfo` and one `MemberName` objects per frame. Some
>> frameworks
>> like logging may only interest in the Class object but not the method name
>> nor the BCI,
>> for example, filters out its implementation classes to find the caller
>> class. It's
>> similar to `StackWalker::getCallerClass` but allows a predicate to filter
>> out the element.
>>
>> This PR proposes to add `Option::DROP_METHOD_INFO` enum that requests to
>> drop the method information. If no method information is needed, a
>> `StackWalker` with `DROP_METHOD_INFO`
>> can be used instead and such stack walker will save the overhead of
>> extracting the method information
>> and the memory used for the stack walking.
>>
>> New factory methods to take a parameter to specify the kind of stack walker
>> to be created are defined.
>> This provides a simple way for existing code, for example logging
>> frameworks, to take advantage of
>> this enhancement with the least change as it can keep the existing function
>> for traversing
>> `StackFrame`s.
>>
>> For example: to find the first caller filtering a known list of
>> implementation class,
>> existing code can create a stack walker instance with `DROP_METHOD_INFO`
>> option:
>>
>>
>> StackWalker walker = StackWalker.getInstance(Option.DROP_METHOD_INFO,
>> Option.RETAIN_CLASS_REFERENCE);
>> Optional> callerClass = walker.walk(s ->
>> s.map(StackFrame::getDeclaringClass)
>> .filter(Predicate.not(implClasses::contains))
>> .findFirst());
>>
>>
>> If method information is accessed on the `StackFrame`s produced by this
>> stack walker such as
>> `StackFrame::getMethodName`, then `UnsupportedOperationException` will be
>> thrown.
>>
>> Javadoc & specdiff
>>
>> https://cr.openjdk.org/~mchung/api/java.base/java/lang/StackWalker.html
>> https://cr.openjdk.org/~mchung/jdk22/specdiff/overview-summary.html
>>
>> Alternatives Considered
>> One alternative is to provide a new API:
>> ` T walkClass(Function, ? extends T> function)`
>>
>> In this case, the caller would need to pass a function that takes a stream
>> of `Class` object instead of `StackFrame`. Existing code would have to
>> modify calls to the `walk` method to `walkClass` and the function body.
>>
>> ### Implementation Details
>>
>> A `StackWalker` configured with `DROP_METHOD_INFO` ...
>
> Mandy Chung has updated the pull request incrementally with one additional
> commit since the last revision:
>
> review feedback
Changes requested by dfuchs (Reviewer).
test/micro/org/openjdk/bench/java/lang/StackWalkBench.java line 60:
> 58: static StackWalker walker(String name) {
> 59: return switch (name) {
> 60: case "class+method" -> WALKER;
don't you need to also change "default" into "class+method" in the
`@Param({"default", "class_only"})` annotations below?
test/micro/org/openjdk/bench/java/lang/StackWalkBench.java line 78:
> 76: public int mark = 4;
> 77:
> 78: @Param({"default", "class_only"})
(I mean here)
-
PR Review: https://git.openjdk.org/jdk/pull/15370#pullrequestreview-1616052545
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1318997721
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1318999463
Re: RFR: 8267174: Many test files have the wrong Copyright header
On Tue, 5 Sep 2023 22:49:41 GMT, Erik Joelsson wrote: > There are a number of files in the `test` directory that have an incorrect > copyright header, which includes the "classpath" exception text. This patch > removes that text from all test files that I could find it in. I did this > using a combination of `sed` and `grep`. Reviewing this patch is probably > easier using the raw patch file or a suitable webrev format. > > It's my assumption that these headers were introduced by mistake as it's > quite easy to copy the wrong template when creating new files. jmx, jndi, and net changes LGTM - PR Review: https://git.openjdk.org/jdk/pull/15573#pullrequestreview-1613147541
Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v8]
On Tue, 29 Aug 2023 20:51:56 GMT, Mandy Chung wrote: >> 8268829: Provide an optimized way to walk the stack with Class object only >> >> `StackWalker::walk` creates one `StackFrame` per frame and the current >> implementation >> allocates one `StackFrameInfo` and one `MemberName` objects per frame. Some >> frameworks >> like logging may only interest in the Class object but not the method name >> nor the BCI, >> for example, filters out its implementation classes to find the caller >> class. It's >> similar to `StackWalker::getCallerClass` but allows a predicate to filter >> out the element. >> >> This PR proposes to add `Option::DROP_METHOD_INFO` enum that requests to >> drop the method information. If no method information is needed, a >> `StackWalker` with `DROP_METHOD_INFO` >> can be used instead and such stack walker will save the overhead of >> extracting the method information >> and the memory used for the stack walking. >> >> New factory methods to take a parameter to specify the kind of stack walker >> to be created are defined. >> This provides a simple way for existing code, for example logging >> frameworks, to take advantage of >> this enhancement with the least change as it can keep the existing function >> for traversing >> `StackFrame`s. >> >> For example: to find the first caller filtering a known list of >> implementation class, >> existing code can create a stack walker instance with `DROP_METHOD_INFO` >> option: >> >> >> StackWalker walker = StackWalker.getInstance(Option.DROP_METHOD_INFO, >> Option.RETAIN_CLASS_REFERENCE); >> Optional> callerClass = walker.walk(s -> >> s.map(StackFrame::getDeclaringClass) >> .filter(Predicate.not(implClasses::contains)) >> .findFirst()); >> >> >> If method information is accessed on the `StackFrame`s produced by this >> stack walker such as >> `StackFrame::getMethodName`, then `UnsupportedOperationException` will be >> thrown. >> >> Javadoc & specdiff >> >> https://cr.openjdk.org/~mchung/api/java.base/java/lang/StackWalker.html >> https://cr.openjdk.org/~mchung/jdk22/specdiff/overview-summary.html >> >> Alternatives Considered >> One alternative is to provide a new API: >> ` T walkClass(Function, ? extends T> function)` >> >> In this case, the caller would need to pass a function that takes a stream >> of `Class` object instead of `StackFrame`. Existing code would have to >> modify calls to the `walk` method to `walkClass` and the function body. >> >> ### Implementation Details >> >> A `StackWalker` configured with `DROP_METHOD_INFO` ... > > Mandy Chung has updated the pull request incrementally with three additional > commits since the last revision: > > - update mode to be int rather than long > - update tests > - Review feedback on javadoc src/java.base/share/classes/java/lang/StackStreamFactory.java line 694: > 692: // no method information is available; should just filter > 693: // "Continuation::yield0". > 694: return classFrames[index].declaringClass() == > Continuation.class; Is that going to be an issue if by chance the frame is some other method on continuation? Could that comment be clarified a bit? I am not sure what is meant by `should just filter "Continuation::yield0"`; Does it mean: that's what we should do, but we can't, so we filter any method on `Continuation` instead? Or does it mean: the only method we expect here if declaringClass is `Continuation` is `yield0`, so the line below should only filter out `Continuation::yield0`? - PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1310422354
Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v7]
On Tue, 29 Aug 2023 16:39:56 GMT, Mandy Chung wrote:
>> src/java.base/share/classes/java/lang/StackWalker.java line 73:
>>
>>> 71: * 1. To find the first caller filtering a known list of
>>> implementation class:
>>> 72: * {@snippet lang="java" :
>>> 73: * StackWalker walker =
>>> StackWalker.getInstance(Option.DROP_METHOD_INFO,
>>> Option.RETAIN_CLASS_REFERENCE);
>>
>> Would this read better as "filtering **out** a known list of implementation
>> **classes**" ?
>
> how about "s/filtering/excluding"?
Yes - that's better.
>> src/java.base/share/classes/java/lang/StackWalker.java line 98:
>>
>>> 96: *
>>> 97: * The information of a {@code StackFrame} available is
>>> determined by the
>>> 98: * {@linkplain Option stack walking options} of a stack walker.
>>
>> Would this read better as "The information available from a {@code
>> StackFrame} is determined ... "?
>
> What about "Stack walker options configure the stack frame information
> obtained by a StackWalker." - the first sentence from the Option javadoc and
> use it in the class spec of StackWalker and StackFrame.
Sounds good!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1309117609
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1309116727
Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v7]
On Tue, 29 Aug 2023 00:16:40 GMT, Mandy Chung wrote:
>> 8268829: Provide an optimized way to walk the stack with Class object only
>>
>> `StackWalker::walk` creates one `StackFrame` per frame and the current
>> implementation
>> allocates one `StackFrameInfo` and one `MemberName` objects per frame. Some
>> frameworks
>> like logging may only interest in the Class object but not the method name
>> nor the BCI,
>> for example, filters out its implementation classes to find the caller
>> class. It's
>> similar to `StackWalker::getCallerClass` but allows a predicate to filter
>> out the element.
>>
>> This PR proposes to add `Option::DROP_METHOD_INFO` enum that requests to
>> drop the method information. If no method information is needed, a
>> `StackWalker` with `DROP_METHOD_INFO`
>> can be used instead and such stack walker will save the overhead of
>> extracting the method information
>> and the memory used for the stack walking.
>>
>> New factory methods to take a parameter to specify the kind of stack walker
>> to be created are defined.
>> This provides a simple way for existing code, for example logging
>> frameworks, to take advantage of
>> this enhancement with the least change as it can keep the existing function
>> for traversing
>> `StackFrame`s.
>>
>> For example: to find the first caller filtering a known list of
>> implementation class,
>> existing code can create a stack walker instance with `DROP_METHOD_INFO`
>> option:
>>
>>
>> StackWalker walker = StackWalker.getInstance(Option.DROP_METHOD_INFO,
>> Option.RETAIN_CLASS_REFERENCE);
>> Optional> callerClass = walker.walk(s ->
>> s.map(StackFrame::getDeclaringClass)
>> .filter(Predicate.not(implClasses::contains))
>> .findFirst());
>>
>>
>> If method information is accessed on the `StackFrame`s produced by this
>> stack walker such as
>> `StackFrame::getMethodName`, then `UnsupportedOperationException` will be
>> thrown.
>>
>> Javadoc & specdiff
>>
>> https://cr.openjdk.org/~mchung/api/java.base/java/lang/StackWalker.html
>> https://cr.openjdk.org/~mchung/jdk22/specdiff/overview-summary.html
>>
>> Alternatives Considered
>> One alternative is to provide a new API:
>> ` T walkClass(Function, ? extends T> function)`
>>
>> In this case, the caller would need to pass a function that takes a stream
>> of `Class` object instead of `StackFrame`. Existing code would have to
>> modify calls to the `walk` method to `walkClass` and the function body.
>>
>> ### Implementation Details
>>
>> A `StackWalker` configured with `DROP_METHOD_INFO` ...
>
> Mandy Chung has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Revised the API change. Add Option::DROP_METHOD_INFO
> - Review feedback from Remi
Hi Mandy, I like the new DROP_METHOD_INFO constant. Just a couple of minor
suggestions on the wording (which you may chose to ignore as English is not my
primary language).
src/java.base/share/classes/java/lang/StackWalker.java line 73:
> 71: * 1. To find the first caller filtering a known list of
> implementation class:
> 72: * {@snippet lang="java" :
> 73: * StackWalker walker =
> StackWalker.getInstance(Option.DROP_METHOD_INFO,
> Option.RETAIN_CLASS_REFERENCE);
Would this read better as "filtering **out** a known list of implementation
**classes**" ?
src/java.base/share/classes/java/lang/StackWalker.java line 98:
> 96: *
> 97: * The information of a {@code StackFrame} available is
> determined by the
> 98: * {@linkplain Option stack walking options} of a stack walker.
Would this read better as "The information available from a {@code StackFrame}
is determined ... "?
-
PR Review: https://git.openjdk.org/jdk/pull/15370#pullrequestreview-1600394012
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1308804787
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1308807710
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v6]
On Tue, 29 Aug 2023 11:58:14 GMT, Sean Coffey wrote: >> Recursive initialization calls possible during loading of LoggerFinder >> service. >> >> This fix detects the recursive call and returns a temporary LoggerFinder >> that is backed by a lazy logger. Automated test case developed to simulate >> loading of an external LoggerFinder service while also having other threads >> poke System.getLogger during this framework initialization. > > Sean Coffey has updated the pull request incrementally with one additional > commit since the last revision: > > Review comments from Daniel. Further test clean up Only minor comment updates. Otherwise LGTM! test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/PlatformRecursiveLoadingTest.java line 46: > 44: > 45: /** > 46: * This test triggers recursion by calling `System.getLogger` in the > class init Suggestion: * This test triggers recursion by calling `System.getLogger` in the class init and constructor test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/RecursiveLoadingTest.java line 41: > 39: > 40: /** > 41: * This test triggers recursion by calling `System.getLogger` in the > class init Suggestion: * This test triggers recursion by calling `System.getLogger` in the class init and constructor - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15404#pullrequestreview-1600360266 PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1308784213 PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1308785012
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v5]
On Mon, 28 Aug 2023 18:49:40 GMT, Daniel Fuchs wrote: >> Sean Coffey has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix up test cases > > test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/PlatformRecursiveLoadingTest.java > line 64: > >> 62: >> logs.stream().map(SimpleLogRecord::of).forEach(System.out::println); >> 63: >> logs.stream().map(SimpleLogRecord::of).forEach(SimpleLogRecord::check); >> 64: assertEquals(String.valueOf(logs.size()), String.valueOf(2)); > > Suggestion: > > assertEquals(String.valueOf(logs.size()), String.valueOf(3)); > > See suggestions to SimpleLoggerFinder below... Creating a logger from within the SimpleLoggerFinder constructor ensures that we get the expected StackOverFlow when the patch is not present. I believe it's a better emulation for the issue that was detected with signed jars, where a logger was created while loading the provider. - PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307798105
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v5]
On Mon, 28 Aug 2023 18:45:03 GMT, Sean Coffey wrote: >> Recursive initialization calls possible during loading of LoggerFinder >> service. >> >> This fix detects the recursive call and returns a temporary LoggerFinder >> that is backed by a lazy logger. Automated test case developed to simulate >> loading of an external LoggerFinder service while also having other threads >> poke System.getLogger during this framework initialization. > > Sean Coffey has updated the pull request incrementally with one additional > commit since the last revision: > > Fix up test cases test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/PlatformRecursiveLoadingTest.java line 64: > 62: > logs.stream().map(SimpleLogRecord::of).forEach(System.out::println); > 63: > logs.stream().map(SimpleLogRecord::of).forEach(SimpleLogRecord::check); > 64: assertEquals(String.valueOf(logs.size()), String.valueOf(2)); Suggestion: assertEquals(String.valueOf(logs.size()), String.valueOf(3)); See suggestions to SimpleLoggerFinder below... - PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307785939
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v4]
On Mon, 28 Aug 2023 17:29:16 GMT, Sean Coffey wrote:
>> Recursive initialization calls possible during loading of LoggerFinder
>> service.
>>
>> This fix detects the recursive call and returns a temporary LoggerFinder
>> that is backed by a lazy logger. Automated test case developed to simulate
>> loading of an external LoggerFinder service while also having other threads
>> poke System.getLogger during this framework initialization.
>
> Sean Coffey has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Tidy up SignedLoggerFinderTest.java
> - Code contribution from Daniel included. New tests. Fix up extra service
> call issues
test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/PlatformRecursiveLoadingTest.java
line 58:
> 56: * of this run). Running test in signed and unsigned jar mode for
> sanity coverage.
> 57: * The current bug only manifests when jars are signed.
> 58: */
Suggestion:
/**
* This test triggers recursion by calling `System.getLogger` in the class
init
* of a custom LoggerFinder. Without the fix, this is expected to throw
* java.lang.NoClassDefFoundError: Could not initialize class
jdk.internal.logger.LoggerFinderLoader$ErrorPolicy
* caused by: java.lang.StackOverflowError
*
*/
test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/PlatformRecursiveLoadingTest.java
line 65:
> 63:
> logs.stream().map(SimpleLogRecord::of).forEach(System.out::println);
> 64:
> logs.stream().map(SimpleLogRecord::of).forEach(SimpleLogRecord::check);
> 65: assertEquals(String.valueOf(logs.size()), String.valueOf(2));*/
Why is this code commented? Is it an oversight?
test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/RecursiveLoadingTest.java
line 55:
> 53: * of this run). Running test in signed and unsigned jar mode for
> sanity coverage.
> 54: * The current bug only manifests when jars are signed.
> 55: */
Suggestion:
/**
* This test triggers recursion by calling `System.getLogger` in the class
init
* of a custom LoggerFinder. Without the fix, this is expected to throw
* java.lang.NoClassDefFoundError: Could not initialize class
jdk.internal.logger.LoggerFinderLoader$ErrorPolicy
* caused by: java.lang.StackOverflowError
*
*/
test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/RecursiveLoadingTest.java
line 61:
> 59:
> logs.stream().map(SimpleLogRecord::of).forEach(System.out::println);
> 60:
> logs.stream().map(SimpleLogRecord::of).forEach(SimpleLogRecord::check);
> 61: assertEquals(String.valueOf(logs.size()), String.valueOf(2));
See suggested changes to SimpleLoggerFinder
Suggestion:
assertEquals(String.valueOf(logs.size()), String.valueOf(3));
test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/SimpleLoggerFinder.java
line 28:
> 26: import java.lang.*;
> 27: import java.util.*;
> 28: import java.util.concurrent.CopyOnWriteArrayList;
Suggestion:
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.ConcurrentHashMap;
test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/SimpleLoggerFinder.java
line 49:
> 47: }
> 48:
> 49: public static final CopyOnWriteArrayList LOGS = new
> CopyOnWriteArrayList<>();
Suggest to move the declaration before the static block above.
test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/SimpleLoggerFinder.java
line 50:
> 48:
> 49: public static final CopyOnWriteArrayList LOGS = new
> CopyOnWriteArrayList<>();
> 50:
Suggestion:
private final Map loggers = new ConcurrentHashMap<>();
public SimpleLoggerFinder() {
System.getLogger("dummy")
.log(System.Logger.Level.INFO,
"Logger finder service created");
}
test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/SimpleLoggerFinder.java
line 53:
> 51: @Override
> 52: public System.Logger getLogger(String name, Module module) {
> 53: return new SimpleLogger(name);
Suggestion:
return loggers.computeIfAbsent(name, SimpleLogger::new);
We're now getting the "dummy" logger twice, and since its constructor has side
effects on its underlying jul logger implementation, we don't want to create
two loggers for the same name - as that would cause two identical SimpleHandler
instances to be added to the same underlying logger, making the log message
appear twice in the LOGS list.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307771237
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307725024
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307773997
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307773194
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307776466
PR Review Comment:
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v4]
On Mon, 28 Aug 2023 17:42:34 GMT, Daniel Fuchs wrote: >> Sean Coffey has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Tidy up SignedLoggerFinderTest.java >> - Code contribution from Daniel included. New tests. Fix up extra service >> call issues > > test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/PlatformRecursiveLoadingTest.java > line 65: > >> 63: >> logs.stream().map(SimpleLogRecord::of).forEach(System.out::println); >> 64: >> logs.stream().map(SimpleLogRecord::of).forEach(SimpleLogRecord::check); >> 65: assertEquals(String.valueOf(logs.size()), String.valueOf(2));*/ > > Why is this code commented? Is it an oversight? BTW: With the modification suggested below in SimpleLoggerFinder, note that the expected number of messages should be 3 (not 2). - PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307772571
Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v6]
On Fri, 25 Aug 2023 22:22:43 GMT, Mandy Chung wrote: >> 8268829: Provide an optimized way to walk the stack with Class object only >> >> `StackWalker::walk` creates one `StackFrame` per frame and the current >> implementation >> allocates one `StackFrameInfo` and one `MemberName` objects per frame. Some >> frameworks >> like logging may only interest in the Class object but not the method name >> nor the BCI, >> for example, filters out its implementation classes to find the caller >> class. It's >> similar to `StackWalker::getCallerClass` but allows a predicate to filter >> out the element. >> >> This PR proposes to add `StackWalker.Kind` enum to specify the information >> that a stack walker >> collects. If no method information is needed, a `StackWalker` of >> `CLASS_INFO` can be used >> instead and such stack walker will save the overhead (1) to extract the >> method information >> and (2) the memory used for the stack walking. In addition, this can also >> fix >> >> - [8311500](https://bugs.openjdk.org/browse/JDK-8311500): >> StackWalker.getCallerClass() throws UOE if invoked reflectively >> >> New factory methods to take a parameter to specify the kind of stack walker >> to be created are defined. >> This provides a simple way for existing code, for example logging >> frameworks, to take advantage of >> this enhancement with the least change as it can keep the existing function >> for traversing >> `StackFrame`s. >> >> For example: to find the first caller filtering a known list of >> implementation class, >> existing code can call `StackWalker::getInstance(CLASS_INFO, ...)` to create >> a stack walker instance: >> >> >> StackWalker walker = StackWalker.getInstance(Kind.CLASS_INFO, >> Option.RETAIN_CLASS_REFERENCE); >> Optional> callerClass = walker.walk(s -> >> s.map(StackFrame::getDeclaringClass) >> .filter(interestingClasses::contains) >> .findFirst()); >> >> >> If method information is accessed on the `StackFrame`s produced by this >> stack walker such as >> `StackFrame::getMethodName`, then `UnsupportedOperationException` will be >> thrown. >> >> Javadoc & specdiff >> >> https://cr.openjdk.org/~mchung/api/java.base/java/lang/StackWalker.html >> https://cr.openjdk.org/~mchung/jdk22/specdiff/overview-summary.html >> >> Alternatives Considered >> One alternative is to provide a new API: >> ` T walkClass(Function, ? extends T> function)` >> >> In this case, the caller would need to pass a function that takes a stream >> of `Class` object instead of `StackFrame`. Existing code would hav... > > Mandy Chung has updated the pull request incrementally with two additional > commits since the last revision: > > - fixup javadoc > - Review feedback: move JLIA to ClassFrameInfo As far as I understand the point of this change is to provide a new API that will allow a caller to request class information only, thus improving performance when only class information is required. Whether we use a new enum or a new option constant, we will need changes to the API documentation. If we use a new option constant we won't need any new static factory methods though. Whether filtering is performed in java or in native is orthogonal to that. The trend these days is to do as much as possible in java, and the patch goes in that direction. There is a small price to pay if the frame to be filtered is a continuation, as this will cause the member name to be resolved. Is that significant enough to push back towards doing the filtering in native: I don't know. It may be worth investigating. - PR Comment: https://git.openjdk.org/jdk/pull/15370#issuecomment-1695899325
Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v4]
On Wed, 23 Aug 2023 20:27:38 GMT, Mandy Chung wrote:
>> 8268829: Provide an optimized way to walk the stack with Class object only
>>
>> `StackWalker::walk` creates one `StackFrame` per frame and the current
>> implementation
>> allocates one `StackFrameInfo` and one `MemberName` objects per frame. Some
>> frameworks
>> like logging may only interest in the Class object but not the method name
>> nor the BCI,
>> for example, filters out its implementation classes to find the caller
>> class. It's
>> similar to `StackWalker::getCallerClass` but allows a predicate to filter
>> out the element.
>>
>> This PR proposes to add `StackWalker.Kind` enum to specify the information
>> that a stack walker
>> collects. If no method information is needed, a `StackWalker` of
>> `CLASS_INFO` can be used
>> instead and such stack walker will save the overhead (1) to extract the
>> method information
>> and (2) the memory used for the stack walking. In addition, this can also
>> fix
>>
>> - [8311500](https://bugs.openjdk.org/browse/JDK-8311500):
>> StackWalker.getCallerClass() throws UOE if invoked reflectively
>>
>> New factory methods to take a parameter to specify the kind of stack walker
>> to be created are defined.
>> This provides a simple way for existing code, for example logging
>> frameworks, to take advantage of
>> this enhancement with the least change as it can keep the existing function
>> for traversing
>> `StackFrame`s.
>>
>> For example: to find the first caller filtering a known list of
>> implementation class,
>> existing code can call `StackWalker::getInstance(CLASS_INFO, ...)` to create
>> a stack walker instance:
>>
>>
>> StackWalker walker = StackWalker.getInstance(Kind.CLASS_INFO,
>> Option.RETAIN_CLASS_REFERENCE);
>> Optional> callerClass = walker.walk(s ->
>> s.map(StackFrame::getDeclaringClass)
>> .filter(interestingClasses::contains)
>> .findFirst());
>>
>>
>> If method information is accessed on the `StackFrame`s produced by this
>> stack walker such as
>> `StackFrame::getMethodName`, then `UnsupportedOperationException` will be
>> thrown.
>>
>> Alternatives Considered
>> One alternative is to provide a new API:
>> ` T walkClass(Function, ? extends T> function)`
>>
>> In this case, the caller would need to pass a function that takes a stream
>> of `Class` object instead of `StackFrame`. Existing code would have to
>> modify calls to the `walk` method to `walkClass` and the function body.
>>
>> Another alternative is to add a new `NO_METHOD_INFO` option. Similar to the
>> proposed API,
>>...
>
> Mandy Chung has updated the pull request incrementally with three additional
> commits since the last revision:
>
> - fix whitespace
> - move retainClassRef to ClassFrameInfo as a bit set in the flags field
> - fixup the factory methods
src/java.base/share/classes/java/lang/ClassFrameInfo.java line 32:
> 30: class ClassFrameInfo implements StackFrame {
> 31: protected Object classOrMemberName;// Class or ResolvedMemberName
> initialized by VM
> 32: protected int flags;
I see only one place where flags is written to and that's at line 35 below. I
guess the reason it's not final is that it can be mutated by the VM. Maybe that
would deserve a comment here:
```suggestion
protected int flags; // flags can be mutated by the VM to indicate hidden
frame
src/java.base/share/classes/java/lang/StackStreamFactory.java line 1083:
> 1081: private static boolean filterStackWalkImpl(Class c) {
> 1082: return stackWalkImplClasses.contains(c) ||
> 1083: c.getPackageName().equals("java.util.stream");
There is a small semantic difference here and in the change below compared to
the original code: the original code would have also included sub-packages,
where the new code will not. Since neither `java.util.stream` nor
`java.lang.invoke` have sub-package I guess it's of no concern for now.
src/java.base/share/classes/java/lang/StackStreamFactory.java line 1096:
> 1094:c == Constructor.class ||
> 1095:MethodAccessor.class.isAssignableFrom(c) ||
> 1096:ConstructorAccessor.class.isAssignableFrom(c);
I guess LambdaForm have the hidden flag on, which is why there's no need to
include them here now?
src/java.base/share/classes/java/lang/StackWalker.java line 333:
> 331: * {@linkplain StackFrame#getMethodType() method type},
> 332: * {@linkplain StackFrame#getLineNumber() line number} and
> 333: * {@linkplain StackFrame#getByteCodeIndex() bytecode index}.
Maybe you should include `getFileName` (and possibly `isNativeMethod`) here?
src/java.base/share/classes/java/lang/StackWalker.java line 338:
> 336: }
> 337:
> 338: enum ExtendedOption {
Should `ExtendedOption` now be renamed into `ExtendedKind`?
test/jdk/java/lang/StackWalker/SanityTest.java line 120:
> 118:
Re: RFR: 8314759: VirtualThread.parkNanos timeout adjustment when pinned should be replaced
On Wed, 23 Aug 2023 16:41:23 GMT, Alan Bateman wrote: > If yielding fails due to the pinning then VirtualThread.parkNanos parks on > the carrier thread with the remaining time. The calculation of the remaining > time needs to be replaced so that it obviously uses the difference between > the start and end time in the calculation. The current code isn't correct for > cases where System.nanoTimes return a negative value or when parking for > durations close to Long.MAX_VALUE (292 years). The change isn't really > testable so there aren't any test changes included. Although... what happens if parkOnCarrierThread is called with a negative value (as that could happen here)? - PR Comment: https://git.openjdk.org/jdk/pull/15405#issuecomment-1691537547
Re: RFR: 8314759: VirtualThread.parkNanos timeout adjustment when pinned should be replaced
On Wed, 23 Aug 2023 16:41:23 GMT, Alan Bateman wrote: > If yielding fails due to the pinning then VirtualThread.parkNanos parks on > the carrier thread with the remaining time. The calculation of the remaining > time needs to be replaced so that it obviously uses the difference between > the start and end time in the calculation. The current code isn't correct for > cases where System.nanoTimes return a negative value or when parking for > durations close to Long.MAX_VALUE (292 years). The change isn't really > testable so there aren't any test changes included. LGTM - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15405#pullrequestreview-1593413969
Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v3]
On Tue, 22 Aug 2023 19:01:53 GMT, Mandy Chung wrote: >> 8268829: Provide an optimized way to walk the stack with Class object only >> >> `StackWalker::walk` creates one `StackFrame` per frame and the current >> implementation >> allocates one `StackFrameInfo` and one `MemberName` objects per frame. Some >> frameworks >> like logging may only interest in the Class object but not the method name >> nor the BCI, >> for example, filters out its implementation classes to find the caller >> class. It's >> similar to `StackWalker::getCallerClass` but allows a predicate to filter >> out the element. >> >> This PR proposes to add `StackWalker.Kind` enum to specify the information >> that a stack walker >> collects. If no method information is needed, a `StackWalker` of >> `CLASS_INFO` can be used >> instead and such stack walker will save the overhead (1) to extract the >> method information >> and (2) the memory used for the stack walking. In addition, this can also >> fix >> >> - [8311500](https://bugs.openjdk.org/browse/JDK-8311500): >> StackWalker.getCallerClass() throws UOE if invoked reflectively >> >> New factory methods to take a parameter to specify the kind of stack walker >> to be created are defined. >> This provides a simple way for existing code, for example logging >> frameworks, to take advantage of >> this enhancement with the least change as it can keep the existing function >> for traversing >> `StackFrame`s. >> >> For example: to find the first caller filtering a known list of >> implementation class, >> existing code can call `StackWalker::getInstance(CLASS_INFO, ...)` to create >> a stack walker instance: >> >> >> StackWalker walker = StackWalker.getInstance(Kind.CLASS_INFO, >> Option.RETAIN_CLASS_REFERENCE); >> Optional> callerClass = walker.walk(s -> >> s.map(StackFrame::getDeclaringClass) >> .filter(interestingClasses::contains) >> .findFirst()); >> >> >> If method information is accessed on the `StackFrame`s produced by this >> stack walker such as >> `StackFrame::getMethodName`, then `UnsupportedOperationException` will be >> thrown. >> >> Alternatives Considered >> One alternative is to provide a new API: >> ` T walkClass(Function, ? extends T> function)` >> >> In this case, the caller would need to pass a function that takes a stream >> of `Class` object instead of `StackFrame`. Existing code would have to >> modify calls to the `walk` method to `walkClass` and the function body. >> >> Another alternative is to add a new `NO_METHOD_INFO` option. Similar to the >> proposed API, >>... > > Mandy Chung has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 16 commits: > > - Replace NO_METHOD_INFO option with StackWalker.Kind enums > - Merge branch 'master' of https://github.com/openjdk/jdk into > stackwalker-class > - clean up > - fix trailing whitespace > - Merge branch 'master' of https://github.com/openjdk/jdk into > stackwalker-class > - Update LocalLongHelper.java > - clean up > - Merge branch 'master' of https://github.com/openjdk/jdk into > stackwalker-class > - Add StackWalker.Option.NO_METHOD_INFO > - Merge branch 'master' of https://github.com/openjdk/jdk into > stackwalker-class > - ... and 6 more: https://git.openjdk.org/jdk/compare/ce1ded1a...434d90cb src/java.base/share/classes/java/lang/StackFrameInfo.java line 38: > 36: SharedSecrets.getJavaLangInvokeAccess(); > 37: > 38: private final boolean retainClassRef; Shouldn't `private final boolean retainClassRef` move to ClassFrameInfo? Isn't it strange that CLASS_INFO still implies RETAIN_CLASS_REFERENCE? - PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1303378176
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError
On Wed, 23 Aug 2023 15:41:16 GMT, Sean Coffey wrote:
> Recursive initialization calls possible during loading of LoggerFinder
> service.
>
> This fix detects the recursive call and returns a temporary LoggerFinder that
> is backed by a lazy logger. Automated test case developed to simulate loading
> of an external LoggerFinder service while also having other threads poke
> System.getLogger during this framework initialization.
src/java.base/share/classes/jdk/internal/logger/BootstrapLogger.java line 230:
> 228: // The accessor in which this logger is temporarily set.
> 229: final LazyLoggerAccessor holder;
> 230: final BooleanSupplier isLoadingThread;
Suggestion:
// tests whether the logger is invoked by the loading thread before
// the LoggerFinder is loaded; can be null;
final BooleanSupplier isLoadingThread;
src/java.base/share/classes/jdk/internal/logger/BootstrapLogger.java line 234:
> 232: boolean isLoadingThread() {
> 233: return isLoadingThread != null && isLoadingThread.getAsBoolean();
> 234: }
Suggestion:
// returns true if the logger is invoked by the loading thread before the
// LoggerFinder service is loaded
boolean isLoadingThread() {
return isLoadingThread != null && isLoadingThread.getAsBoolean();
}
src/java.base/share/classes/jdk/internal/logger/BootstrapLogger.java line 948:
> 946: // and the LogManager has not been initialized yet.
> 947: public static boolean useLazyLoggers() {
> 948: if (!BootstrapLogger.isBooted() ||
Suggestion:
// Note: avoid triggering the initialization of the DetectBackend class
// while holding the BotstrapLogger class monitor
if (!BootstrapLogger.isBooted() ||
src/java.base/share/classes/jdk/internal/logger/LazyLoggers.java line 425:
> 423: */
> 424: public static final Logger getLogger(String name, Module module) {
> 425: BootstrapLogger.detectBackend();
Suggestion:
// triggers detection of the backend
BootstrapLogger.detectBackend();
src/java.base/share/classes/jdk/internal/logger/LoggerFinderLoader.java line 76:
> 74:
> 75: // Return the loaded LoggerFinder, or load it if not already loaded.
> 76: static volatile Thread loadingThread;
Suggestion:
// record the loadingThread while loading the backend
static volatile Thread loadingThread;
// Return the loaded LoggerFinder, or load it if not already loaded.
src/java.base/share/classes/jdk/internal/logger/LoggerFinderLoader.java line 83:
> 81: Thread currentThread = Thread.currentThread();
> 82: if (loadingThread == currentThread) {
> 83: return new TemporaryLoggerFinder();
Suggestion:
// recursive ttempt to load the backend while loading the
backend
// use a temporary logger finder that returns special
BootsrtapLoggers
// which will wait until loading is finished
return new TemporaryLoggerFinder();
src/java.base/share/classes/jdk/internal/logger/LoggerFinderLoader.java line 98:
> 96: }
> 97:
> 98: static boolean isLoadingThread() {
Suggestion:
// returns true if called by the thread that loads the LoggerFinder, while
// loading the LoggerFinder.
static boolean isLoadingThread() {
src/java.base/share/classes/jdk/internal/logger/LoggerFinderLoader.java line
140:
> 138: }
> 139:
> 140: static class TemporaryLoggerFinder extends LoggerFinder {
Suggestion:
private static final class TemporaryLoggerFinder extends LoggerFinder {
src/java.base/share/classes/jdk/internal/logger/LoggerFinderLoader.java line
145:
> 143: @Override
> 144: public Logger apply(String name, Module module) {
> 145: return LazyLoggers.getLoggerFromFinder(name,
> module);
A better idea might be to:
Suggestion:
return LazyLoggers.getLogger(name, module);
src/java.base/share/classes/jdk/internal/logger/LoggerFinderLoader.java line
154:
> 152: }
> 153: };
> 154:
Suggestion:
private static final TemporaryLoggerFinder INSTANCE = new
TemporaryLoggerFinder();
test/jdk/java/lang/System/LoggerFinder/SignedLoggerFinderTest/SignedLoggerFinderTest.java
line 161:
> 159: Logger testLogger = Logger.getLogger("jdk.event.security");
> 160: assertEquals(testLogger.getClass().getName(),
> "java.util.logging.Logger");
> 161: testComplete = true;
I believe these lines should be after `Security.setProperty("test_2",
"test");`, to make sure the logger is loaded by the module that uses it. Also I
was expecting some kind of checks to verify that the expected messages are
indeed logged.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303306422
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303310327
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError
On Wed, 23 Aug 2023 17:15:03 GMT, Daniel Fuchs wrote:
>> Recursive initialization calls possible during loading of LoggerFinder
>> service.
>>
>> This fix detects the recursive call and returns a temporary LoggerFinder
>> that is backed by a lazy logger. Automated test case developed to simulate
>> loading of an external LoggerFinder service while also having other threads
>> poke System.getLogger during this framework initialization.
>
> src/java.base/share/classes/jdk/internal/logger/LoggerFinderLoader.java line
> 83:
>
>> 81: Thread currentThread = Thread.currentThread();
>> 82: if (loadingThread == currentThread) {
>> 83: return new TemporaryLoggerFinder();
>
> Suggestion:
>
> // recursive ttempt to load the backend while loading the
> backend
> // use a temporary logger finder that returns special
> BootsrtapLoggers
> // which will wait until loading is finished
> return new TemporaryLoggerFinder();
We could create a singleton instance of TemporaryLoggerFinder in the
TemporaryLoggerFinder class and return that.
Suggestion:
return new TemporaryLoggerFinder.INSTANCE;
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303321618
Re: RFR: 8313768: Reduce interaction with volatile field in j.u.l.StreamHandler
On Fri, 4 Aug 2023 14:51:35 GMT, Sergey Tsypanov wrote: > In `publish0()`, `flush0()` and `flushAndClose()`methods of `StreamHandler` > we read multiple times from volatile writer. The access number can be reduced > by reading the field into local variable once. LGTM - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15161#pullrequestreview-1567302875
Re: [jdk21] RFR: 8300937: Update nroff pages in JDK 21 before RC
On Mon, 31 Jul 2023 08:33:07 GMT, David Holmes wrote: > Main changes are to use 21 instead of 21-ea. In addition the following files > contain additional updates from the closed sources: > > - src/java.base/share/man/java.1 > > [JDK-8273131](https://bugs.openjdk.org/browse/JDK-8273131): Update the java > manpage markdown source for JFR filename expansion > [JDK-8221633](https://bugs.openjdk.org/browse/JDK-8221633): > StartFlightRecording: consider moving mention of multiple comma-separated > parameters to the front > > There are also some formatting differences related to: > > [JDK-8309670](https://bugs.openjdk.org/browse/JDK-8309670): java -help > output for --module-path / -p is incomplete > > Likely a different version of pandoc was used. > > - src/java.base/share/man/keytool.1 > > [JDK-8290626](https://bugs.openjdk.org/browse/JDK-8290626): keytool manpage > contains a special character > > - src/jdk.compiler/share/man/javac.1 > > [JDK-8308456](https://bugs.openjdk.org/browse/JDK-8308456): Update javac > tool page for -proc:full > > also some formatting differences. > > - src/jdk.jartool/share/man/jarsigner.1 > > [JDK-8303928](https://bugs.openjdk.org/browse/JDK-8303928): Update jarsigner > man page after [JDK-8303410](https://bugs.openjdk.org/browse/JDK-8303410) > > - src/jdk.jcmd/share/man/jcmd.1 > > [JDK-8273131](https://bugs.openjdk.org/browse/JDK-8273131): Update the java > manpage markdown source for JFR filename expansion > > - src/jdk.jdeps/share/man/jdeps.1 > > [JDK-8301207](https://bugs.openjdk.org/browse/JDK-8301207): (jdeps) > Deprecate jdeps -profile option > > - src/jdk.jfr/share/man/jfr.1 > > Formatting changes. > > - src/jdk.jshell/share/man/jshell.1 > > [JDK-8308988](https://bugs.openjdk.org/browse/JDK-8308988): Update JShell > manpage to include TOOLING description Changes to jwebserver.1 look good to me. - PR Comment: https://git.openjdk.org/jdk21/pull/151#issuecomment-1658469731
Re: RFR: 8310892: ScopedValue throwing StructureViolationException should be clearer [v3]
On Mon, 10 Jul 2023 05:05:13 GMT, Alan Bateman wrote: >> Docs only update to add a missing `@throws StructureViolationException` and >> make it clearer when the exception is thrown. In the future we might >> re-visit this so that the description is in one place rather than in each >> method. > > Alan Bateman 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 > - s/ ith/with/ > - Initial commit LGTM - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14679#pullrequestreview-1521829170
Integrated: 8310987: Missing @since tag(s) in java/util/logging/ErrorManager.java
On Fri, 30 Jun 2023 09:06:20 GMT, Daniel Fuchs wrote: > Please find here a trivial doc fix to add a missing `@since 1.4` to the > java.util.logging.ErrorManager class. This pull request has now been integrated. Changeset: e8ff74c7 Author:Daniel Fuchs URL: https://git.openjdk.org/jdk/commit/e8ff74c7e84ec2440a51fee1b4c45e87332807a0 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod 8310987: Missing @since tag(s) in java/util/logging/ErrorManager.java Reviewed-by: lancea, iris - PR: https://git.openjdk.org/jdk/pull/14725
RFR: 8310987: Missing @since tag(s) in java/util/logging/ErrorManager.java
Please find here a trivial doc fix to add a missing `@since 1.4` to the java.util.logging.ErrorManager class. - Commit messages: - 8310987 Changes: https://git.openjdk.org/jdk/pull/14725/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14725=00 Issue: https://bugs.openjdk.org/browse/JDK-8310987 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14725.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14725/head:pull/14725 PR: https://git.openjdk.org/jdk/pull/14725
