Re: RFR: 8324672: Update jdk/java/time/tck/java/time/TCKInstant.java now() to be more robust [v2]

[Daniel Fuchs]

On Thu, 10 Oct 2024 19:42:14 GMT, Roger Riggs  wrote:

>> I am keeping the timeout as 60 seconds. is this ok?
>> 
>> @Test(timeOut=6)
>> public void now() {
>> Instant expected, test;
>> long diff;
>> do {
>> expected = Instant.now(Clock.systemUTC());
>> test = Instant.now();
>> diff = Math.abs(test.toEpochMilli() - expected.toEpochMilli());
>> } while( diff > 100 ); // retry if more than 0.1 sec
>> }
>
> Yes, looks fine;  The normal JTREG timeout is 2 minutes. 60 seconds is fine 
> for the testng timeout.

FWIW - when I updated the System UTC clock to get sub-milliseconds resolution 
from the OS I added this test:
https://github.com/openjdk/jdk/blob/master/test/jdk/java/time/test/java/time/TestClock_System.java

Maybe some similar technique could be used here. That is - take 
System.currentTimeMillis(), Take Instant.now(), take System.currentTimeMillis() 
again, and then verify that the instant lies between the two snapshots: greater 
or equal to the first, less or equal to the second. That should always be true 
(unless the UTC clock is adjusted by NTP). But you could hopefully detect that 
and retry if you observe that the second call to System.currentTimeMillis() has 
returned a value which is before the first call. 

If the difference between the two System::curentTimeMillis calls is too big, 
then if you wish you might want to try again too.

I believe this would provide a more robust test strategy.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/21413#discussion_r1797211143

Re: RFR: 8341789: Fix ExceptionOccurred in java.base

[Daniel Fuchs]

On Wed, 9 Oct 2024 17:28:09 GMT, Justin Lu  wrote:

> Please review this PR which fixes incorrect usage of `jthrowable 
> ExceptionOccurred(JNIEnv *env)` within _java.base_.
> 
> This corrects instances where the return value is being treated as a boolean. 
> Such occurrences are replaced with `jboolean ExceptionCheck(JNIEnv *env)`.

Marked as reviewed by dfuchs (Reviewer).

>From 
>https://docs.oracle.com/en/java/javase/23/docs/specs/jni/functions.html#exceptioncheck
> :

> ExceptionCheck
> We introduce a convenience function to check for pending exceptions without 
> creating a local reference to the exception object.
> 
> jboolean ExceptionCheck(JNIEnv *env);
> 
> Returns JNI_TRUE when there is a pending exception; otherwise, returns 
> JNI_FALSE.
> 

So this looks good to me too.

-

PR Review: https://git.openjdk.org/jdk/pull/21428#pullrequestreview-2359643144
PR Comment: https://git.openjdk.org/jdk/pull/21428#issuecomment-2404578043

Re: RFR: 8339538: Wrong timeout computations in DnsClient [v11]

[Daniel Fuchs]

On Mon, 7 Oct 2024 09:00:28 GMT, Aleksei Efimov  wrote:

>> This PR proposes the following changes to address wrong timeout computations 
>> in the `com.sun.jndi.dns.DnsClient`:
>> - The `DnsClient` has been updated to use a monotonic high-resolution (nano) 
>> clock. The existing `Timeout` test has also been updated to use the nano 
>> clock to measure observed timeout value.
>> 
>> - The left timeout computation has been fixed to decrease the timeout value 
>> during each retry attempt. A new test, `TimeoutWithEmptyDatagrams`, has been 
>> added to test it.
>> 
>> - The `DnsClient.blockingReceive` has been updated:
>> - to detect if any data is received
>> - to avoid contention with `Selector.close()` that could be called by a 
>> cleaner thread
>> 
>> - The expected timeout calculation in the `Timeout` test has been updated to 
>> take into account the minimum retry timeout (50ms). Additionally, the max 
>> allowed difference between the observed timeout and the expected one has 
>> been increased from 50% to 67%. Taking into account 50 ms retry timeout 
>> decrease the maximum allowed difference is effectively set to 61%. This 
>> change is expected to improve the stability of the `Timeout` test which has 
>> been seen to fail 
>> [intermittentlly](https://bugs.openjdk.org/browse/JDK-8220213). If no 
>> objections, I'm planning to close 
>> [JDK-8220213](https://bugs.openjdk.org/browse/JDK-8220213) as duplicate of 
>> this one.
>> 
>> JNDI/DNS jtreg tests has been executed multiple times (500+) to check if the 
>> new and the modified tests are stable. No failures been observed (so far?).
>
> Aleksei Efimov 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 16 additional 
> commits since the last revision:
> 
>  - Merge remote-tracking branch 'origin' into 
> JDK-8339538_DnsClient_nanoTime_And_InfiniteLoop
>  - improve reporting for unrecoverable exceptions
>  - Add comment suggested by Daniel
>  - Track unfulfilled timeouts during UDP queries.
>Update exceptions handling.
>Update TCP client to use nano time source.
>  - set min timeout to 0; set max allowed timeout to 2x expected timeout in 
> tests
>  -  set max allowed value for retries to 30
>  - update tests to calculate allowed timeout range (max is updated to 75%), 
> print it and use it for elapsed time check
>  - remove redundant clamp from timeoutLeft calculation
>  - clamp timeout and retries property values
>  - Measure time the caller spent waiting. Simplify timeoutLeft computation
>  - ... and 6 more: https://git.openjdk.org/jdk/compare/f0646048...c58e34cc

Marked as reviewed by dfuchs (Reviewer).

Thanks Aleksei. I believe reporting the exception that causes to switch 
`doNotRetry[i]` to `true` when there is a single server is the right thing to 
do... In case of several servers, there isn't any obvious choice since the 
other servers could eventually give a valid response, or timeout... In that 
case adding the exception to a possibly pre-existing (TimeoutException?) with 
addSuppressed() looks like the best choice.

-

PR Review: https://git.openjdk.org/jdk/pull/20892#pullrequestreview-2351664712
PR Comment: https://git.openjdk.org/jdk/pull/20892#issuecomment-2396649911

Re: RFR: 8339538: Wrong timeout computations in DnsClient [v9]

[Daniel Fuchs]

On Wed, 2 Oct 2024 13:35:00 GMT, Aleksei Efimov  wrote:

>> This PR proposes the following changes to address wrong timeout computations 
>> in the `com.sun.jndi.dns.DnsClient`:
>> - The `DnsClient` has been updated to use a monotonic high-resolution (nano) 
>> clock. The existing `Timeout` test has also been updated to use the nano 
>> clock to measure observed timeout value.
>> 
>> - The left timeout computation has been fixed to decrease the timeout value 
>> during each retry attempt. A new test, `TimeoutWithEmptyDatagrams`, has been 
>> added to test it.
>> 
>> - The `DnsClient.blockingReceive` has been updated:
>> - to detect if any data is received
>> - to avoid contention with `Selector.close()` that could be called by a 
>> cleaner thread
>> 
>> - The expected timeout calculation in the `Timeout` test has been updated to 
>> take into account the minimum retry timeout (50ms). Additionally, the max 
>> allowed difference between the observed timeout and the expected one has 
>> been increased from 50% to 67%. Taking into account 50 ms retry timeout 
>> decrease the maximum allowed difference is effectively set to 61%. This 
>> change is expected to improve the stability of the `Timeout` test which has 
>> been seen to fail 
>> [intermittentlly](https://bugs.openjdk.org/browse/JDK-8220213). If no 
>> objections, I'm planning to close 
>> [JDK-8220213](https://bugs.openjdk.org/browse/JDK-8220213) as duplicate of 
>> this one.
>> 
>> JNDI/DNS jtreg tests has been executed multiple times (500+) to check if the 
>> new and the modified tests are stable. No failures been observed (so far?).
>
> Aleksei Efimov has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add comment suggested by Daniel

Marked as reviewed by dfuchs (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/20892#pullrequestreview-2342978749

Re: RFR: 8340326: Remove references to Applet in core-libs/security tests [v7]

[Daniel Fuchs]

On Fri, 27 Sep 2024 18:08:54 GMT, Justin Lu  wrote:

>> Please review this PR which removes usages of Applet within the corelibs 
>> tests.
>> 
>> Most changes are removed comments/updated var names. The JBS issue lists 
>> more files than the ones included in this pull request, please see the 
>> comment on the JBS issue for the reason why they were not included.
>> 
>> The following files were removed,
>> 
>> - test/jdk/java/util/TimeZone/DefaultTimeZoneTest.html
>>   - Test runner is no longer an Applet, so not needed
>> - test/jdk/java/net/Socket/SocketImplTest.java
>>   - An old Applet test missing Jtreg tags.
>
> Justin Lu has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   revert bugID update to tests

Since Naoto previously approved before the simple `@bug` updates I am going to 
approve now. @prrace no objection?

-

Marked as reviewed by dfuchs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21096#pullrequestreview-2341048410

Re: RFR: 8339850: Restore the interrupt status in FileSystemPreferences.lockFile() [v2]

[Daniel Fuchs]

On Mon, 30 Sep 2024 14:29:03 GMT, sbgoog  wrote:

>> In which case the code might be simplified to just:
>> 
>> } catch (InterruptedException e) {
>> // Don't lose the interrupt
>> Thread.currentThread().interrupt();
>> break;
>> }
>
> I've reworked the change to always set the interrupted status. I wouldn't 
> remove the check of the error code here, as it'd be a behavior change. I can 
> follow up with that, though it seems to me that it's good to still have the 
> check for access error here.

My suggestion was to `break;` instead of `return false;` which would fall 
through to line 967 where the check method would be called. But what you have 
is probably preferable as it preserves the original style and keep the changes 
minimal.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20938#discussion_r1781313496

Re: RFR: 8340572: ConcurrentModificationException when sorting ArrayList sublists

[Daniel Fuchs]

On Sun, 29 Sep 2024 17:44:30 GMT, Attila Szegedi  wrote:

> Fixes a regression with #17818 where `ArrayList.subList(…).sort()` started 
> incrementing `ArrayList.modCount` resulting in some cases throwing a 
> `ConcurrentModificationException` where none was thrown before.
> 
> This change keeps the optimization from #17818 but restores the behavior 
> where only sorting the `ArrayList` changes the mod count, but sorting its 
> sublists does not.

I am not a spcialist here. But if sorting the parent list is considered as a 
modification, and if the sublist is just a view of the parent list, then surely 
sorting/modifying the sublist should be considered as a modification of the 
parent list? 

In which case the issue here is probably deeper: SubList and parent list appear 
to maintain separate `modcount` variables which are only loosely coupled. Maybe 
that's the bug that ought to be fixed?

-

PR Comment: https://git.openjdk.org/jdk/pull/21250#issuecomment-2383407392

Re: RFR: 8339850: Restore the interrupt status in FileSystemPreferences.lockFile()

[Daniel Fuchs]

On Mon, 30 Sep 2024 13:46:55 GMT, Daniel Jeliński  wrote:

>> I thought that if an access error is found, then that would take precedence 
>> over the interrupt, especially since it occurs before the sleep. I was more 
>> concerned with just returning false and the caller not knowing if it's false 
>> due to interrupt, or false because the file could not be locked after all 
>> retries.
>> 
>> Certainly the interrupt status could move before the check which may 
>> through, but I think it's less likely that the status will be checked in a 
>> catch of SecurityException.
>
> The status might not be explicitly checked, but setting the interrupted 
> status will make sure that subsequent calls to sleep/await/tryLock etc. will 
> not block.
> 
> In general, we want to preserve the interrupted status until either the user 
> decides that it's fine to clear, or until the thread dies.

In which case the code might be simplified to just:

} catch (InterruptedException e) {
// Don't lose the interrupt
Thread.currentThread().interrupt();
break;
}

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20938#discussion_r1781202892

Re: RFR: 8339538: Wrong timeout computations in DnsClient [v8]

[Daniel Fuchs]

On Thu, 19 Sep 2024 17:55:13 GMT, Aleksei Efimov  wrote:

>> This PR proposes the following changes to address wrong timeout computations 
>> in the `com.sun.jndi.dns.DnsClient`:
>> - The `DnsClient` has been updated to use a monotonic high-resolution (nano) 
>> clock. The existing `Timeout` test has also been updated to use the nano 
>> clock to measure observed timeout value.
>> 
>> - The left timeout computation has been fixed to decrease the timeout value 
>> during each retry attempt. A new test, `TimeoutWithEmptyDatagrams`, has been 
>> added to test it.
>> 
>> - The `DnsClient.blockingReceive` has been updated:
>> - to detect if any data is received
>> - to avoid contention with `Selector.close()` that could be called by a 
>> cleaner thread
>> 
>> - The expected timeout calculation in the `Timeout` test has been updated to 
>> take into account the minimum retry timeout (50ms). Additionally, the max 
>> allowed difference between the observed timeout and the expected one has 
>> been increased from 50% to 67%. Taking into account 50 ms retry timeout 
>> decrease the maximum allowed difference is effectively set to 61%. This 
>> change is expected to improve the stability of the `Timeout` test which has 
>> been seen to fail 
>> [intermittentlly](https://bugs.openjdk.org/browse/JDK-8220213). If no 
>> objections, I'm planning to close 
>> [JDK-8220213](https://bugs.openjdk.org/browse/JDK-8220213) as duplicate of 
>> this one.
>> 
>> JNDI/DNS jtreg tests has been executed multiple times (500+) to check if the 
>> new and the modified tests are stable. No failures been observed (so far?).
>
> Aleksei Efimov has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Track unfulfilled timeouts during UDP queries.
>   Update exceptions handling.
>   Update TCP client to use nano time source.

This looks good to me. I believe this should lead to the least surprising 
behavior to the user, where we wait at least for the expected timeout.

-

Marked as reviewed by dfuchs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20892#pullrequestreview-2337639659

Re: RFR: 8339538: Wrong timeout computations in DnsClient [v8]

[Daniel Fuchs]

On Thu, 19 Sep 2024 17:55:13 GMT, Aleksei Efimov  wrote:

>> This PR proposes the following changes to address wrong timeout computations 
>> in the `com.sun.jndi.dns.DnsClient`:
>> - The `DnsClient` has been updated to use a monotonic high-resolution (nano) 
>> clock. The existing `Timeout` test has also been updated to use the nano 
>> clock to measure observed timeout value.
>> 
>> - The left timeout computation has been fixed to decrease the timeout value 
>> during each retry attempt. A new test, `TimeoutWithEmptyDatagrams`, has been 
>> added to test it.
>> 
>> - The `DnsClient.blockingReceive` has been updated:
>> - to detect if any data is received
>> - to avoid contention with `Selector.close()` that could be called by a 
>> cleaner thread
>> 
>> - The expected timeout calculation in the `Timeout` test has been updated to 
>> take into account the minimum retry timeout (50ms). Additionally, the max 
>> allowed difference between the observed timeout and the expected one has 
>> been increased from 50% to 67%. Taking into account 50 ms retry timeout 
>> decrease the maximum allowed difference is effectively set to 61%. This 
>> change is expected to improve the stability of the `Timeout` test which has 
>> been seen to fail 
>> [intermittentlly](https://bugs.openjdk.org/browse/JDK-8220213). If no 
>> objections, I'm planning to close 
>> [JDK-8220213](https://bugs.openjdk.org/browse/JDK-8220213) as duplicate of 
>> this one.
>> 
>> JNDI/DNS jtreg tests has been executed multiple times (500+) to check if the 
>> new and the modified tests are stable. No failures been observed (so far?).
>
> Aleksei Efimov has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Track unfulfilled timeouts during UDP queries.
>   Update exceptions handling.
>   Update TCP client to use nano time source.

src/jdk.naming.dns/share/classes/com/sun/jndi/dns/DnsClient.java line 247:

> 245: continue;
> 246: }
> 247: AtomicLong unfulfilledServerTimeout = 
> unfulfilledUdpTimeouts[i];

Suggestion:

// unfulfilledServerTimeout is always >= 0
AtomicLong unfulfilledServerTimeout = 
unfulfilledUdpTimeouts[i];

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20892#discussion_r1781153536

Re: RFR: 8340326: Remove references to Applet in core-libs/security tests [v7]

[Daniel Fuchs]

On Fri, 27 Sep 2024 18:08:54 GMT, Justin Lu  wrote:

>> Please review this PR which removes usages of Applet within the corelibs 
>> tests.
>> 
>> Most changes are removed comments/updated var names. The JBS issue lists 
>> more files than the ones included in this pull request, please see the 
>> comment on the JBS issue for the reason why they were not included.
>> 
>> The following files were removed,
>> 
>> - test/jdk/java/util/TimeZone/DefaultTimeZoneTest.html
>>   - Test runner is no longer an Applet, so not needed
>> - test/jdk/java/net/Socket/SocketImplTest.java
>>   - An old Applet test missing Jtreg tags.
>
> Justin Lu has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   revert bugID update to tests

The changes that restore the original `@bug` in tests look good to me

-

PR Comment: https://git.openjdk.org/jdk/pull/21096#issuecomment-2383151330

Re: RFR: 8340326: Remove references to Applet in core-libs/security tests [v6]

[Daniel Fuchs]

On Thu, 26 Sep 2024 17:24:18 GMT, Justin Lu  wrote:

>> Please review this PR which removes usages of Applet within the corelibs 
>> tests.
>> 
>> Most changes are removed comments/updated var names. The JBS issue lists 
>> more files than the ones included in this pull request, please see the 
>> comment on the JBS issue for the reason why they were not included.
>> 
>> The following files were removed,
>> 
>> - test/jdk/java/util/TimeZone/DefaultTimeZoneTest.html
>>   - Test runner is no longer an Applet, so not needed
>> - test/jdk/java/net/Socket/SocketImplTest.java
>>   - An old Applet test missing Jtreg tags.
>
> Justin Lu has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   restore ParseUtil_6380332.java test

Logging and sun/net changes look reasonable to me.

-

PR Review: https://git.openjdk.org/jdk/pull/21096#pullrequestreview-2331962137

Re: RFR: 8340326: Remove references to Applet in core-libs/security tests [v5]

[Daniel Fuchs]

On Thu, 26 Sep 2024 17:00:18 GMT, Justin Lu  wrote:

>> Please review this PR which removes usages of Applet within the corelibs 
>> tests.
>> 
>> Most changes are removed comments/updated var names. The JBS issue lists 
>> more files than the ones included in this pull request, please see the 
>> comment on the JBS issue for the reason why they were not included.
>> 
>> The following files were removed,
>> 
>> - test/jdk/java/util/TimeZone/DefaultTimeZoneTest.html
>>   - Test runner is no longer an Applet, so not needed
>> - test/jdk/sun/net/www/ParseUtil_6380332.java
>>   - Outdated test that checks a SunTea applet
>> - test/jdk/java/net/Socket/SocketImplTest.java
>>   - An old Applet test missing Jtreg tags.
>
> Justin Lu has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   review: replace last control panel instance with Settings app

`test/jdk/sun/net/www/ParseUtil_6380332.java`: shouldn't we actually keep this 
test? I see that the code in ParseUtil which is tested here is still there, and 
I don't think we'd want to touch ParseUtil given potential compatibility issues 
that might arise.

-

PR Comment: https://git.openjdk.org/jdk/pull/21096#issuecomment-2377495252

Re: Integrated: 8340956: ProblemList 4 java/nio/channels/DatagramChannel tests on macosx-all

[Daniel Fuchs]

On Wed, 25 Sep 2024 17:07:57 GMT, Daniel D. Daugherty  
wrote:

> A trivial fix to ProblemList 4 java/nio/channels/DatagramChannel tests on 
> macosx-all.

Marked as reviewed by dfuchs (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/21188#pullrequestreview-2328965652

Re: RFR: 8337143: (fc, fs) Move filesystem-related native objects from libnio to libjava [v5]

[Daniel Fuchs]

On Fri, 13 Sep 2024 16:35:04 GMT, Brian Burkhalter  wrote:

>> Do we know what code loaded NTLMAuthentication? I'd expect it to be loaded 
>> by HttpURLConnection, which should have triggered the loading of libnet long 
>> before it cares about NTLM.
>
> I would have to check. The failure I observed occurred in both of these tests
> 
> test/jdk/sun/net/www/protocol/http/NoNTLM.java
> test/jdk/sun/net/www/protocol/http/TestTransparentNTLM.java
> 
> and nowhere else.

I see. The test does:

Class ntlmProxyClass = 
Class.forName("sun.net.www.protocol.http.NTLMAuthenticationProxy", true, 
NoNTLM.class.getClassLoader());

so that explains it.

In this case I believe it's fair enough to have NTLMAuthentication trigger the 
loading of libnet if not loaded already -since we need that library to perform 
the class initialization properly.

What you have is good to me.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20317#discussion_r1759183851

Re: RFR: 8337143: (fc, fs) Move filesystem-related native objects from libnio to libjava [v5]

[Daniel Fuchs]

On Fri, 13 Sep 2024 16:12:28 GMT, Brian Burkhalter  wrote:

>> Yes, there was an `UnsatisfiedLinkError` in the native method 
>> `isTrustedSiteAvailable()` in `NTLMAuthentication`.
>
>> [...] I'd expect that libnet would have been loaded before we reach 
>> NTLMAuthentication.
> 
> In the (as of now) penultimate webrev 4f47d5a (Merge), 
> `WindowsNativeDispatcher` loaded it during boot phase 2. I think that without 
> these changes it is likely loaded by `IOUtil`.

Do we know what code loaded NTLMAuthentication? I'd expect it to be loaded by 
HttpURLConnection, which should have triggered the loading of libnet long 
before it cares about NTLM.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20317#discussion_r1759158214

Re: RFR: 8337143: (fc, fs) Move filesystem-related native objects from libnio to libjava [v5]

[Daniel Fuchs]

On Fri, 13 Sep 2024 16:05:37 GMT, Daniel Fuchs  wrote:

>> @dfuch Would you please check whether the change at line 71 in the updated 
>> version of `NTLMAuthentication` is the correct place for this load?
>
> hmm...  I don't see any issue in adding the load call to `NTLMAuthentication` 
> but I'm surprised that it's even needed: I'd expect that libnet would have 
> been loaded before we reach NTLMAuthentication.

I wonder - do you see any failure if you don't load libnet from there?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20317#discussion_r1759125047

Re: RFR: 8337143: (fc, fs) Move filesystem-related native objects from libnio to libjava [v5]

[Daniel Fuchs]

On Thu, 12 Sep 2024 15:40:27 GMT, Brian Burkhalter  wrote:

>> Moved to `NTLMAuthentication` static initializer. 853d349
>
> @dfuch Would you please check whether the change at line 71 in the updated 
> version of `NTLMAuthentication` is the correct place for this load?

hmm...  I don't see any issue in adding the load call to `NTLMAuthentication` 
but I'm surprised that it's even needed: I'd expect that libnet would have been 
loaded before we reach NTLMAuthentication.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20317#discussion_r1759124071

Re: RFR: 8339538: Wrong timeout computations in DnsClient [v5]

[Daniel Fuchs]

On Tue, 10 Sep 2024 19:01:54 GMT, Mark Sheppard  wrote:

>> I agree that we don't want to document too much here. Updated the factor to 
>> 1.75 (2 seems a bit high and might hide real issues), and to make the 
>> timeout value calculation and check less arcane - I have updated test output 
>> to print the range of acceptable timeout values: 
>> 05ed9e053865293a1938ed7bc6fe208759513813
>
> 2 time  is not too high,
> I have presented, in the comment, a failures with the elapsed time is almost 
> twice the expected time
> where the elapsed time is 14229 !! which is approx 1.84 * expected timeout

@msheppar with the latest proposed change to set MIN_TIMEOUT to 0, then I 
believe the `@implNote` you suggested is no longer needed.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20892#discussion_r1755135496

Re: RFR: 8339538: Wrong timeout computations in DnsClient [v7]

[Daniel Fuchs]

On Wed, 11 Sep 2024 15:22:43 GMT, Aleksei Efimov  wrote:

>> This PR proposes the following changes to address wrong timeout computations 
>> in the `com.sun.jndi.dns.DnsClient`:
>> - The `DnsClient` has been updated to use a monotonic high-resolution (nano) 
>> clock. The existing `Timeout` test has also been updated to use the nano 
>> clock to measure observed timeout value.
>> 
>> - The left timeout computation has been fixed to decrease the timeout value 
>> during each retry attempt. A new test, `TimeoutWithEmptyDatagrams`, has been 
>> added to test it.
>> 
>> - The `DnsClient.blockingReceive` has been updated:
>> - to detect if any data is received
>> - to avoid contention with `Selector.close()` that could be called by a 
>> cleaner thread
>> 
>> - The expected timeout calculation in the `Timeout` test has been updated to 
>> take into account the minimum retry timeout (50ms). Additionally, the max 
>> allowed difference between the observed timeout and the expected one has 
>> been increased from 50% to 67%. Taking into account 50 ms retry timeout 
>> decrease the maximum allowed difference is effectively set to 61%. This 
>> change is expected to improve the stability of the `Timeout` test which has 
>> been seen to fail 
>> [intermittentlly](https://bugs.openjdk.org/browse/JDK-8220213). If no 
>> objections, I'm planning to close 
>> [JDK-8220213](https://bugs.openjdk.org/browse/JDK-8220213) as duplicate of 
>> this one.
>> 
>> JNDI/DNS jtreg tests has been executed multiple times (500+) to check if the 
>> new and the modified tests are stable. No failures been observed (so far?).
>
> Aleksei Efimov has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - set min timeout to 0; set max allowed timeout to 2x expected timeout in 
> tests
>  -  set max allowed value for retries to 30

+1 for changing MIN_TIMEOUT to 0. I don't understand what the original intent 
was when the 50ms value was introduced.

-

PR Comment: https://git.openjdk.org/jdk/pull/20892#issuecomment-2344117302

Re: RFR: 8339538: Wrong timeout computations in DnsClient [v6]

[Daniel Fuchs]

On Tue, 10 Sep 2024 18:41:41 GMT, Aleksei Efimov  wrote:

>> This PR proposes the following changes to address wrong timeout computations 
>> in the `com.sun.jndi.dns.DnsClient`:
>> - The `DnsClient` has been updated to use a monotonic high-resolution (nano) 
>> clock. The existing `Timeout` test has also been updated to use the nano 
>> clock to measure observed timeout value.
>> 
>> - The left timeout computation has been fixed to decrease the timeout value 
>> during each retry attempt. A new test, `TimeoutWithEmptyDatagrams`, has been 
>> added to test it.
>> 
>> - The `DnsClient.blockingReceive` has been updated:
>> - to detect if any data is received
>> - to avoid contention with `Selector.close()` that could be called by a 
>> cleaner thread
>> 
>> - The expected timeout calculation in the `Timeout` test has been updated to 
>> take into account the minimum retry timeout (50ms). Additionally, the max 
>> allowed difference between the observed timeout and the expected one has 
>> been increased from 50% to 67%. Taking into account 50 ms retry timeout 
>> decrease the maximum allowed difference is effectively set to 61%. This 
>> change is expected to improve the stability of the `Timeout` test which has 
>> been seen to fail 
>> [intermittentlly](https://bugs.openjdk.org/browse/JDK-8220213). If no 
>> objections, I'm planning to close 
>> [JDK-8220213](https://bugs.openjdk.org/browse/JDK-8220213) as duplicate of 
>> this one.
>> 
>> JNDI/DNS jtreg tests has been executed multiple times (500+) to check if the 
>> new and the modified tests are stable. No failures been observed (so far?).
>
> Aleksei Efimov has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - update tests to calculate allowed timeout range (max is updated to 75%), 
> print it and use it for elapsed time check
>  - remove redundant clamp from timeoutLeft calculation
>  - clamp timeout and retries property values

src/jdk.naming.dns/share/classes/com/sun/jndi/dns/DnsContext.java line 185:

> 183: int val = Integer.parseInt((String) propVal);
> 184: if (retries != val) {
> 185: retries = Math.clamp(val, 1, 31);

Maybe we should rather use 30 here, since `1 << 1` is 2 and `1 << 31` is 
negative

src/jdk.naming.dns/share/classes/com/sun/jndi/dns/DnsContext.java line 264:

> 262: retries = (val == null)
> 263: ? DEFAULT_RETRIES
> 264: : Math.clamp(Integer.parseInt(val), 1, 31);

same remark herre

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20892#discussion_r1753832166
PR Review Comment: https://git.openjdk.org/jdk/pull/20892#discussion_r1753833767

Re: RFR: 8339538: Wrong timeout computations in DnsClient [v5]

[Daniel Fuchs]

On Tue, 10 Sep 2024 09:49:08 GMT, Mark Sheppard  wrote:

>> Aleksei Efimov has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Measure time the caller spent waiting. Simplify timeoutLeft computation
>
> test/jdk/com/sun/jndi/dns/ConfigTests/Timeout.java line 112:
> 
>> 110: // Check that elapsed time is as long as expected, and
>> 111: // not more than 67% greater. Given the min DNS timeout
>> 112: // correction above the threshold value is equal to 61%.
> 
> this is a bit arcane, why not have a simple measure that elapsed time 
> shouldn't be more than twice the expected timeout ... this is not that 
> different to the  multipliedBy(2) and multipliedBy(3) --  
> elaspedTime.compareTo(expectedTime.multipliedBy(2) <= 0
> 
> Additionally based on the internal minimum timeout allowance of 50 secs and 
> this upper bound calculation, it would suggest that an @implNote might be 
> useful, or required, to alert developers  to potential timeout variability, 
> and not to rely on timeout to be absolutely (real time) precise

I don't think we want to go down the rabbit hole of documenting too much. 
Agreed that using a simple factor 2 would make the code simpler, but do we want 
to go that high?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20892#discussion_r1752155232

Re: RFR: 8339538: Wrong timeout computations in DnsClient [v5]

[Daniel Fuchs]

On Mon, 9 Sep 2024 22:29:23 GMT, Aleksei Efimov  wrote:

>> This PR proposes the following changes to address wrong timeout computations 
>> in the `com.sun.jndi.dns.DnsClient`:
>> - The `DnsClient` has been updated to use a monotonic high-resolution (nano) 
>> clock. The existing `Timeout` test has also been updated to use the nano 
>> clock to measure observed timeout value.
>> 
>> - The left timeout computation has been fixed to decrease the timeout value 
>> during each retry attempt. A new test, `TimeoutWithEmptyDatagrams`, has been 
>> added to test it.
>> 
>> - The `DnsClient.blockingReceive` has been updated:
>> - to detect if any data is received
>> - to avoid contention with `Selector.close()` that could be called by a 
>> cleaner thread
>> 
>> - The expected timeout calculation in the `Timeout` test has been updated to 
>> take into account the minimum retry timeout (50ms). Additionally, the max 
>> allowed difference between the observed timeout and the expected one has 
>> been increased from 50% to 67%. Taking into account 50 ms retry timeout 
>> decrease the maximum allowed difference is effectively set to 61%. This 
>> change is expected to improve the stability of the `Timeout` test which has 
>> been seen to fail 
>> [intermittentlly](https://bugs.openjdk.org/browse/JDK-8220213). If no 
>> objections, I'm planning to close 
>> [JDK-8220213](https://bugs.openjdk.org/browse/JDK-8220213) as duplicate of 
>> this one.
>> 
>> JNDI/DNS jtreg tests has been executed multiple times (500+) to check if the 
>> new and the modified tests are stable. No failures been observed (so far?).
>
> Aleksei Efimov has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Measure time the caller spent waiting. Simplify timeoutLeft computation

src/jdk.naming.dns/share/classes/com/sun/jndi/dns/DnsClient.java line 479:

> 477: long elapsedMillis = TimeUnit.NANOSECONDS
> 478:  
> .toMillis(System.nanoTime() - start);
> 479: timeoutLeft = pktTimeout - Math.clamp(elapsedMillis, 
> 0, Integer.MAX_VALUE);

Suggestion:

timeoutLeft = pktTimeout - elapsedMillis;

Now that timeoutLeft is a long we have no reason to clamp at Integer.MAX_VALUE.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20892#discussion_r1752147242

Re: RFR: 8339538: Wrong timeout computations in DnsClient [v5]

[Daniel Fuchs]

On Tue, 10 Sep 2024 08:39:15 GMT, Daniel Jeliński  wrote:

>> Aleksei Efimov has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Measure time the caller spent waiting. Simplify timeoutLeft computation
>
> src/jdk.naming.dns/share/classes/com/sun/jndi/dns/DnsClient.java line 443:
> 
>> 441: // integer overflow (timeout is an int).
>> 442: // no point in supporting timeout > Integer.MAX_VALUE, 
>> clamp if needed
>> 443: long pktTimeout = Math.clamp(timeout * (1L << retry), 
>> 0, Integer.MAX_VALUE);
> 
> we may also need to clamp retry to [0,31]. or even less.

Agreed - though I don't think it matter much. If the shift overflows to 
negative `pktTimeout` would become 0 which would be interpreted as wait forever.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20892#discussion_r1752134498

Re: RFR: 8339538: Wrong timeout computations in DnsClient [v5]

[Daniel Fuchs]

On Tue, 10 Sep 2024 09:38:35 GMT, Mark Sheppard  wrote:

>> Aleksei Efimov has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Measure time the caller spent waiting. Simplify timeoutLeft computation
>
> src/jdk.naming.dns/share/classes/com/sun/jndi/dns/DnsClient.java line 442:
> 
>> 440: // use 1L below to ensure conversion to long and avoid 
>> potential
>> 441: // integer overflow (timeout is an int).
>> 442: // no point in supporting timeout > Integer.MAX_VALUE, 
>> clamp if needed
> 
> if I have read this correctly,  timeout is of type int, thus int 
> Math.clamp(int, int, int) is being called returning type int and promoting to 
> long. Are there any side effects to consider here?  And as timeoutLeft (or 
> remainingTimeout) and pktTimeout were both int and now is type long, then why 
> have timeout declared as type  int ? 
> 
> consistency in various declared "timeout" variables' type ?

If I'm not mistaken here  it's `Math.clamp(long, long, long)` which is called - 
because `timeout * (1L << retry)` is a long. We could make it more obvious by 
using `0L` instead of `0` too.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20892#discussion_r1752129677

Re: RFR: 8339538: Wrong timeout computations in DnsClient [v2]

[Daniel Fuchs]

On Mon, 9 Sep 2024 11:42:25 GMT, Aleksei Efimov  wrote:

>> This PR proposes the following changes to address wrong timeout computations 
>> in the `com.sun.jndi.dns.DnsClient`:
>> - The `DnsClient` has been updated to use a monotonic high-resolution (nano) 
>> clock. The existing `Timeout` test has also been updated to use the nano 
>> clock to measure observed timeout value.
>> 
>> - The left timeout computation has been fixed to decrease the timeout value 
>> during each retry attempt. A new test, `TimeoutWithEmptyDatagrams`, has been 
>> added to test it.
>> 
>> - The `DnsClient.blockingReceive` has been updated:
>> - to detect if any data is received
>> - to avoid contention with `Selector.close()` that could be called by a 
>> cleaner thread
>> 
>> - The expected timeout calculation in the `Timeout` test has been updated to 
>> take into account the minimum retry timeout (50ms). Additionally, the max 
>> allowed difference between the observed timeout and the expected one has 
>> been increased from 50% to 67%. Taking into account 50 ms retry timeout 
>> decrease the maximum allowed difference is effectively set to 61%. This 
>> change is expected to improve the stability of the `Timeout` test which has 
>> been seen to fail 
>> [intermittentlly](https://bugs.openjdk.org/browse/JDK-8220213). If no 
>> objections, I'm planning to close 
>> [JDK-8220213](https://bugs.openjdk.org/browse/JDK-8220213) as duplicate of 
>> this one.
>> 
>> JNDI/DNS jtreg tests has been executed multiple times (500+) to check if the 
>> new and the modified tests are stable. No failures been observed (so far?).
>
> Aleksei Efimov has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - guard against possible integer value overflows
>  - make startTime a local variable

src/jdk.naming.dns/share/classes/com/sun/jndi/dns/DnsClient.java line 440:

> 438: InetSocketAddress target = new InetSocketAddress(server, 
> port);
> 439: udpChannel.connect(target);
> 440: long pktTimeout = (timeout * (1L << retry));

Suggestion:

// use 1L below to ensure conversion to long and avoid potential
// integer overflow (timeout is an int) 
long pktTimeout = (timeout * (1L << retry));

src/jdk.naming.dns/share/classes/com/sun/jndi/dns/DnsClient.java line 444:

> 442: 
> 443: // timeout remaining after successive 'blockingReceive()'
> 444: long timeoutLeft = Math.clamp(pktTimeout, 0, 
> Integer.MAX_VALUE);

Suggestion:

// no point in supporting timeout > Integer.MAX_VALUE, clamp if 
needed
long timeoutLeft = Math.clamp(pktTimeout, 0, Integer.MAX_VALUE);

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20892#discussion_r1750444825
PR Review Comment: https://git.openjdk.org/jdk/pull/20892#discussion_r1750447201

Re: RFR: 8339538: Wrong timeout computations in DnsClient [v2]

[Daniel Fuchs]

On Mon, 9 Sep 2024 14:42:41 GMT, Aleksei Efimov  wrote:

>> src/jdk.naming.dns/share/classes/com/sun/jndi/dns/DnsClient.java line 477:
>> 
>>> 475: long elapsedMillis = 
>>> TimeUnit.NANOSECONDS.toMillis(end - start);
>>> 476: // Setting the Math.clamp min to 1 ensures that 
>>> the timeout is decreased
>>> 477: timeoutLeft = timeoutLeft - 
>>> Math.clamp(elapsedMillis, 1, Integer.MAX_VALUE);
>> 
>> I think I'd prefer to calculate the remaining timeout based on the total 
>> elapsed time in this loop, as opposed to the time spent in blockingReceive.
>
> Sounds like a right thing to do:  measuring time in the loop should give us 
> better estimation on time DNS client spends waiting on the response after 
> submiting a query (that's how environment property value is defined in 
> [javadoc 
> here](https://docs.oracle.com/en/java/javase/22/docs/api/jdk.naming.dns/module-summary.html)).
> I've tried to move `start` and `end` like:
> 
>  do {
> +long start = System.nanoTime();
> <>
> -long start = System.nanoTime();
>  gotData = blockingReceive(udpChannel, ipkt, timeoutLeft);
> -long end = System.nanoTime();
> <...>
> +long end = System.nanoTime();
>  long elapsedMillis = TimeUnit.NANOSECONDS.toMillis(end - 
> start);
> 
> As a result the tests showed that the timeout happened with a bit better 
> precision (10th of milliseconds).
> I will run more tests and incorporate the suggestions. Thank you.

I have been wondering why the timeout was measured in this way. My explanation 
was that the original author of the API (duke? ;-) ) wanted to measure "actual 
timeout" (time actually spent waiting for a packet) as opposed to perceived 
timeout (time the caller spent waiting). Rationale might have been to exclude 
potential time spent waiting for a lock before entering receive() - for 
instance. That said - I'm not opposed to extend the scope of the timeout to 
make it closer to the perceived timeout, which is what the test expect. 
If we do this maybe we could move `start` out of the `do` block  - which would 
make the computation of `timeoutLeft` simpler (we wouldn't need the `-=`).

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20892#discussion_r1750438221

Re: RFR: 8339687: Rearrange reachabilityFence()s in jdk.test.lib.util.ForceGC

[Daniel Fuchs]

On Sat, 7 Sep 2024 00:40:24 GMT, Stuart Marks  wrote:

>> From the bug description:
>> ForceGC would be improved by moving the Reference.reachabilityFence() calls 
>> for 'obj' and 'ref'.
>> 
>> Reference.reachabilityFence(obj) is currently placed after 'obj' has been 
>> set to null, so effectively does nothing. It should occur before obj = null;
>> 
>> For Reference.reachabilityFence(ref): 'ref' is a PhantomReference to 'obj', 
>> and is registered with 'queue'. ForceGC.waitFor() later remove()s the 
>> reference from the queue, as an indication that some GC and reference 
>> processing has taken place (hopefully causing the BooleanSupplier to return 
>> true).
>> 
>> The code expects the PhantomReference to be cleared and be put on the queue. 
>> But recall that a Reference refers to its queue, and not the other way 
>> around. If a Reference becomes unreachable and is garbage collected, it will 
>> never be enqueued.
>> 
>> I argue that the VM/GC could determine that 'ref' is not used by waitFor() 
>> and collect it before the call to queue.remove(). Moving 
>> Reference.reachabilityFence(ref) after the for() loop would prevent this 
>> scenario.
>> 
>> While this is only a very minor deficiency in ForceGC, I believe it would be 
>> good to ensure that the code behaves as expected.
>
> test/lib/jdk/test/lib/util/ForceGC.java line 102:
> 
>> 100: }
>> 101: }
>> 102: Reference.reachabilityFence(ref);
> 
> I think everything from the creation of ref to the line above needs to 
> enclosed in a try-statement, with the finally-clause including RF(ref).

Arguably the same might also apply to the other call to reachability fence: 
that is - we might need two try-finally to keep things by-the-book?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20898#discussion_r1750398105

Re: RFR: 8339687: Rearrange reachabilityFence()s in jdk.test.lib.util.ForceGC

[Daniel Fuchs]

On Mon, 9 Sep 2024 05:01:26 GMT, David Holmes  wrote:

>> test/lib/jdk/test/lib/util/ForceGC.java line 82:
>> 
>>> 80: PhantomReference ref = new PhantomReference<>(obj, 
>>> queue);
>>> 81: Reference.reachabilityFence(obj);
>>> 82: obj = null;
>> 
>> You're right to question the utility of calling reachabilityFence(obj) after 
>> obj has been nulled out. But I'm still questioning the utility of calling 
>> RF(obj) at all. We don't care when obj is determined to be unreachable; what 
>> we care about is that the GC has done some reference processing. Seems to me 
>> we can simplify the above lines to
>> 
>> PhantomReference ref = new PhantomReference<>(new Object(), 
>> queue);
>> 
>> and get rid of the local variable obj entirely.
>
> The reason for the explicit reference and RF, as I recall, is to guard 
> against the allocation of the new object being elided entirely, with the 
> `PhantomReference` constructor being passed null (or itself being elided) and 
> no reference processing ever actually happening.

What David says ;-)

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20898#discussion_r1750394382

Re: RFR: 8339538: Wrong timeout computations in DnsClient

[Daniel Fuchs]

On Fri, 6 Sep 2024 16:28:36 GMT, Aleksei Efimov  wrote:

> This PR proposes the following changes to address wrong timeout computations 
> in the `com.sun.jndi.dns.DnsClient`:
> - The `DnsClient` has been updated to use a monotonic high-resolution (nano) 
> clock. The existing `Timeout` test has also been updated to use the nano 
> clock to measure observed timeout value.
> 
> - The left timeout computation has been fixed to decrease the timeout value 
> during each retry attempt. A new test, `TimeoutWithEmptyDatagrams`, has been 
> added to test it.
> 
> - The `DnsClient.blockingReceive` has been updated:
> - to detect if any data is received
> - to avoid contention with `Selector.close()` that could be called by a 
> cleaner thread
> 
> - The expected timeout calculation in the `Timeout` test has been updated to 
> take into account the minimum retry timeout (50ms). Additionally, the max 
> allowed difference between the observed timeout and the expected one has been 
> increased from 50% to 67%. Taking into account 50 ms retry timeout decrease 
> the maximum allowed difference is effectively set to 61%. This change is 
> expected to improve the stability of the `Timeout` test which has been seen 
> to fail [intermittentlly](https://bugs.openjdk.org/browse/JDK-8220213). If no 
> objections, I'm planning to close 
> [JDK-8220213](https://bugs.openjdk.org/browse/JDK-8220213) as duplicate of 
> this one.
> 
> JNDI/DNS jtreg tests has been executed multiple times (500+) to check if the 
> new and the modified tests are stable. No failures been observed (so far?).

test/jdk/com/sun/jndi/dns/ConfigTests/TimeoutWithEmptyDatagrams.java line 62:

> 60: private static final int DNS_CLIENT_MIN_TIMEOUT = 50;
> 61: 
> 62: private long startTime;

startTime does not need to be an instance variable: consider removing it and 
make it a local variable in the runTest() method.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20892#discussion_r1747445029

Re: [External] : Re: [POTENTIAL BUG] Potential FIFO violation in BlockingQueue under high contention and suggestion for fair mode in ArrayBlockingQueue and LinkedBlockingQueue

[Daniel Fuchs]

Hi Kim,

On 05/09/2024 06:10, 김민주 wrote:
If I use an external lock, T1 will block in the |await()| state, but T2, 
T3, and T4 will be waiting for the external lock rather than entering 
the |await()| state in |put()|. This would prevent me from simulating 
the specific behavior I’m trying to test.


I understand. But my point is that waking up callers in exactly the
same order they have have arrived may not be of much interest since you
would need first to ensure that they arrive in exactly that
proper order.

I’d appreciate your thoughts on whether this behavior (where a newly 
arriving thread can overtake a waiting thread) is expected or if it’s 
worth further investigation. As this is my first attempt to contribute 
to OpenJDK, I’m eager to learn from the process and would love to help 
resolve the issue if necessary.


I am not sure how strong the "fairness" constraint is.

Typically for monitors, when a thread is signaled after the monitor
is released "it competes in the usual manner with other threads for the 
right to synchronize on the object"

https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Object.html#wait(long,int)

That said, we're speakings of locks here - and the only thing I
could find (in ReentrantLock) is that if fairness is set, then
"under contention, locks favor granting access to the
longest-waiting thread", but note that "fairness of locks does
not guarantee fairness of thread scheduling. Thus, one of many
threads using a fair lock may obtain it multiple times in
succession while other active threads are not progressing and
not currently holding the lock."
https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/util/concurrent/locks/ReentrantLock.html

I am not an expert of the java.util.concurrent package, and
hopefully others in this list will be able to provide more
insights.

Also, since English is not my first language, I hope my previous emails 
didn’t come across as rude or unclear. If they did, I sincerely 
apologize, as it was never my intention to be disrespectful. I’m still 
learning how to communicate effectively in this space, and I appreciate 
your understanding and patience.


Hey - you're welcome - and I found your emails quite clear.
But English is not my first language either ;-)

best regards,

-- daniel



Thank you for your time and guidance.

Best regards,

Re: Possible bug in jdk.naming.dns. I need guidance on how get someone smarter to look at it.

[Daniel Fuchs]

On 04/09/2024 15:02, Marko Bakšić wrote:

Thank you Daniel.

The part that was suspicious to me is

```
int timeoutLeft = pktTimeout;
do {
  ...
  timeoutLeft = pktTimeout - ((int) (end - start));
} while (timeoutLeft > MIN_TIMEOUT);
```

Here, timeoutLeft is not iteratively decreased, but is always derived 
from `pktTimeout`.
I can see a case where `timeoutLeft` never drops below `MIN_TIMEOUT` 
(this is the part where I'm not sure if I'm missing some deeper knowledge).


Indeed - good observation!

-- daniel

Re: Possible bug in jdk.naming.dns. I need guidance on how get someone smarter to look at it.

[Daniel Fuchs]

Hi Marko,

This is not the proper list for this kind of question:
I'm moving the discussion to core-libs-dev at openjdk.org.

There's definitely a bug here: that code should use
System.nanoTime() and not System.currentTimeMillis()
since the latter is not guaranteed to be monotonic.

It may not explain the issue you are observing, but
again, it may cause the timeout to punctually increase.

I have logged
https://bugs.openjdk.org/browse/JDK-8339538

I wonder if that has an impact on what you are observing.

best regards,

-- daniel

On 04/09/2024 13:50, Marko Bakšić wrote:

Hey there!

Sorry for coming here with a technical question, but I would appreciate 
if you could point me in the right direction.


We experienced a bug in production a few times already and through 
profiling we suspect it is an infinite loop here:

https://github.com/openjdk/jdk/blob/master/src/jdk.naming.dns/share/classes/com/sun/jndi/dns/DnsClient.java#L475
 


I came here before opening a bug report, because I'm not sure I am 
right. We haven't been able to reproduce the bug in a dev environment.
Before spending much more time trying to reproduce this, I was hoping 
someone smarter could give me a hint if this is even the right direction.


If no one here can help, I'd appreciate if you could point me in a 
direction of somewhere where I could have a casual discussion about 
this, or get some more eyes on this.


Thank you very much.

-- ​Marko




*Marko Bakšić*

Software Engineer



*E *marko.bak...@infobip.com

*M*



*A *Utinjska 29A, 1 Zagreb, Croatia

www.infobip.com



 
 
 	 



*GSMA Associate Member*
This email message and any attachments are intended solely for the use 
of the addressee. If you are not the intended recipient, you are 
prohibited from reading, disclosing, reproducing, distributing, 
disseminating or otherwise using this transmission. If you have received 
this message in error, please promptly notify the sender by reply email 
and immediately delete this message from your system. This message and 
any attachments may contain information that is confidential, privileged 
or exempt from disclosure. Delivery of this message to any person other 
than the intended recipient is not intended to waive any right or privilege.

Re: [External] : Re: [POTENTIAL BUG] Potential FIFO violation in BlockingQueue under high contention and suggestion for fair mode in ArrayBlockingQueue and LinkedBlockingQueue

[Daniel Fuchs]

Hi Kim,

On 04/09/2024 12:50, 김민주 wrote:
In the original approach, I intended for each thread to call |put|, 
confirm that it has entered the waiting state, and then allow the next 
thread to put the next sequence value and also enter the waiting state. 
This approach was designed to simulate a situation where the queue is 
already full and each thread has to wait in line for space to become 
available.


The problem with that is that you would still need to ensure that each
thread calls `put` in order, and for that, you would still need
external synchronization.

I am not an expert in java.util.concurrent, but it feels like the
fix you had in mind would not buy you much.

best regards,

-- daniel

Re: [POTENTIAL BUG] Potential FIFO violation in BlockingQueue under high contention and suggestion for fair mode in ArrayBlockingQueue and LinkedBlockingQueue

[Daniel Fuchs]

Hi Kim,

When several threads compete to put items in a queue,
regardless of queue locking strategies, nothing guarantees
the order in which items will end up in the queue if
the action of obtaining the next element and putting
it in the queue is not atomic. This is because a thread
can be preempted by the scheduler at any time between
two operations.

Consider the following scenario:

you have a sequence of ordered items a, b, c, one queue q,
and two threads T1, T2 and T3:

T1 starts, obtains element a, and gets preempted (the thread
   is paused by the scheduler).
T2 starts, obtains element b and puts it in the queue.
T3 starts, obtains element c and gets preempted.
T1 gets scheduled again and puts a in the queue.
T3 gets scheduled again and puts c in the queue.

result: the queue contains b a c; regardless of whether
the queue uses fair or unfair locking, the order of items
in the queue will be unpredictable. And in this scenario,
there isn't even any contention on the queue.

To ensure that items will end up in the queue in the proper
order would require an additional lock at the application
level.

That is - you would need to use a lock so that taking
an element from the sequenceGenerator and putting it in the
queue becomes atomic (with respect to the queue).

so in your example you would need something like:

static ReentrantLock lock = new ReentrantLock();
...

  Thread thread = new Thread(() -> {
lock.lock();
try {
var token = sequenceGenerator.getAndIncrement();
// even if this thread gets preempted here, no
// other thread will be able to obtain the next
// token before our token has been put in the queue
try {
queue.put(token);
} catch (InterruptedException e) {
// restore the sequence
var restored = sequenceGenerator.decrementAndGet();
assert token == restored;
}
} finally {
lock.unlock();
}

});
thread.start();

best regards,

-- daniel

On 04/09/2024 08:09, 김민주 wrote:
// This configuration ensures items enter the queue's waiting list in a 
predictable sequence (10, 11, 12, ...) // allowing us to verify if they 
are later consumed in the same order for (int i = 0; i < PRODUCER_COUNT; 
i++) { Thread thread = new Thread(() -> { try { 
queue.put(sequenceGenerator.getAndIncrement()); } catch 
(InterruptedException e) { // ignore } });

Re: RFR: 8338445: jdk.internal.loader.URLClassPath may leak JarFile instance when dealing with unexpected Class-Path entry in manifest [v2]

[Daniel Fuchs]

On Mon, 26 Aug 2024 15:17:18 GMT, Michael McMahon  wrote:

>> Jaikiran Pai has updated the pull request incrementally with three 
>> additional commits since the last revision:
>> 
>>  - revert to old code comment
>>  - use "JAR file" consistently in test's code comments
>>  - rename closeLoaderIgnoreEx to closeQuietly
>
> src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 470:
> 
>> 468: continue;
>> 469: }
>> 470: if (loaderClassPathURLs != null) {
> 
> I'm missing something here. Why does the compiler not complain that 
> `loaderClassPathURLs` might not be initialized, when it's accessed here?

Because we would not reach here - since it's either assigned at line 447 or we 
continue at line 457, 468, or exit throwing an uncaught exception?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1731421835

Re: RFR: 8337143: (fc, fs) Move filesystem-related native objects from libnio to libjava [v5]

[Daniel Fuchs]

On Fri, 9 Aug 2024 17:59:12 GMT, Brian Burkhalter  wrote:

>> This proposed change would move the native objects required for NIO file 
>> interaction from the libnio native library to the libjava native library on 
>> Linux, macOS, and Windows.
>
> Brian Burkhalter has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8337143: Remove loading libnet from Inet6AddressImpl; delete commented out 
> #include in Windows IOUtil.c

The changes to the java/net code look OK to me now. Thanks Brian. I am 
approving these changes - but please also get a Reviewer for the NIO and build 
side of these.

-

Marked as reviewed by dfuchs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20317#pullrequestreview-2233341338

Re: RFR: 8337143: (fc, fs) Move filesystem-related native objects from libnio to libjava [v4]

[Daniel Fuchs]

On Fri, 9 Aug 2024 15:09:08 GMT, Brian Burkhalter  wrote:

>> OK, this test uses a private API to create an instance of Inet6AddressImpl; 
>> This explain why in this test Inet6AddressImpl gets loaded before 
>> InetAddress.
>> 
>> var impl = new Inet6AddressImpl();
>> 
>> It should never happen outside of this test. Now the tricky question: what 
>> in your proposed changes caused "net" not to be loaded before the test 
>> created new Inet6AddressImpl(); ?
>
> Loading "net" was removed from IOUtil so I am thinking that IOUtil must have 
> been initialized somewhere before constructing Inet6AddressImpl, but I've not 
> identified where just yet.

I see. Inet6AddressImpl and Inet4AddressImpl are symetric classes implementing 
InetAddressImpl. Both will make native calls to the "net" library - so both 
require the library to be loaded.

In the JDK, these two classes are only loaded by InetAddress, after the "net" 
library has been loaded.

The test test java/net/InetAddress/NullCharInHostnameDriver.java seems to be an 
outlier (@AlekseiEfimov ?) which for some reason uses a private API (the test 
is injected in java.base) and create a new instance of Inet6AddressImpl before 
InetAddress is loaded. This is why without this change to Inet6AddressImpl we 
get the UnsatisfiedLinkError. We would have gotten the same from 
Inet4AddressImpl if the test had first required access to Inet4AddressImpl 
instead.

So it seems that we should either add the call to load the "net" library to 
both Inet6AddressImpl and Inet4AddressImpl for symetry (there doesn't seem to 
be any reason why Inet6AddressImpl would be preferred in this respect),  or 
remove this call from Inet6AddressImpl and add it to the test instead. Adding 
the call to jdk.internal.loader.BootLoader.loadLibrary("net"); in the test 
should work since the actual test class (NullCharInHostname.java) is injected 
in java.base.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20317#discussion_r1711706176

Re: RFR: 8337143: (fc, fs) Move filesystem-related native objects from libnio to libjava [v4]

[Daniel Fuchs]

On Thu, 8 Aug 2024 21:23:58 GMT, Brian Burkhalter  wrote:

>> Thanks for the suggestions. I will look into it.
>
> Without loading libnet in Inet6AddressImpl, the test 
> java/net/InetAddress/NullCharInHostnameDriver.java fails with 
> UnsatisfiedLinkError:
> 
> java.lang.UnsatisfiedLinkError: 'java.net.InetAddress[] 
> java.net.Inet6AddressImpl.lookupAllHostAddr(java.lang.String, int)'
>   at java.base/java.net.Inet6AddressImpl.lookupAllHostAddr(Native Method)
>   at 
> java.base/java.net.Inet6AddressImpl.lookupAllHostAddr(Inet6AddressImpl.java:52)
>   at 
> java.base/java.net.NullCharInHostname.main(NullCharInHostname.java:37)
> 
> This is on Unix (Linux, macOS) only, not on Windows.

OK, this test uses a private API to create an instance of Inet6AddressImpl; 
This explain why in this test Inet6AddressImpl gets loaded before InetAddress.

var impl = new Inet6AddressImpl();

It should never happen outside of this test. Now the tricky question: what in 
your proposed changes caused "net" not to be loaded before the test created new 
Inet6AddressImpl(); ?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20317#discussion_r1711100272

Re: RFR: 8337143: (fc, fs) Move filesystem-related native objects from libnio to libjava [v4]

[Daniel Fuchs]

On Thu, 8 Aug 2024 16:21:30 GMT, Brian Burkhalter  wrote:

>> I don't know - you added that code to Inet6AddressImpl - so presumably a 
>> test was failing without that code?
>> Which test was that? It wasn't obvious to me that adding code to load the 
>> "net" library would be required here.
>
> I'll have to investigate.

Possibly - if you made isIPv6Supported() in InetAddress.c return false, you 
might be able to see the issue in the same test that you observed failing 
without your change. 

InetAddress has a static block that will load the "net" library, and an other 
static block that will create the InetAddressImpl. It's a bit curious because I 
would expect the block that loads the "net" library to be executed before.

And the only place where I see Inet6AddressImpl being used is in that second 
static block in InetAddress.

I am not an expert on library loading though :-(

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20317#discussion_r1709897049

Re: RFR: 8337143: (fc, fs) Move filesystem-related native objects from libnio to libjava [v4]

[Daniel Fuchs]

On Thu, 8 Aug 2024 16:09:55 GMT, Brian Burkhalter  wrote:

>> It may be because we have no IPv4 only machine in the CI? It seems strange 
>> that IPv6 is treated differently than IPv4 in that respect.
>
> How would you suggest testing this?

I don't know - you added that code to Inet6AddressImpl - so presumably a test 
was failing without that code?
Which test was that? It wasn't obvious to me that adding code to load the "net" 
library would be required here.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20317#discussion_r1709861717

Re: RFR: 8337143: (fc, fs) Move filesystem-related native objects from libnio to libjava [v4]

[Daniel Fuchs]

On Thu, 8 Aug 2024 14:32:25 GMT, Brian Burkhalter  wrote:

>> src/java.base/share/classes/java/net/Inet6AddressImpl.java line 154:
>> 
>>> 152: static {
>>> 153: jdk.internal.loader.BootLoader.loadLibrary("net");
>>> 154: }
>> 
>> I am curious about this change - wouldn't it be needed in Inet4AddressImpl 
>> as well?
>
> I have not seen any failures in CI testing. Is there a specific test that 
> would reveal whether this is a problem?

It may be because we have no IPv4 only machine in the CI? It seems strange that 
IPv6 is treated differently than IPv4 in that respect.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20317#discussion_r1709742606

Re: RFR: 8336926: jdk/internal/util/ReferencedKeyTest.java can fail with ConcurrentModificationException

[Daniel Fuchs]

On Wed, 7 Aug 2024 19:26:59 GMT, Roger Riggs  wrote:

> The original test fails intermittently, the reproducer failed consistently.
> With the fix, the failure was not observed in the test or reproducer.
> 
> In jdk.internal.util.ReferencedKeyMap.entrySet() and toString() methods, 
> avoid removing stale references that would modify the keyset while it is 
> being iterated over. 
> If GC removes the key, iterating or streaming over the keyset might get a 
> ConcurrentModificationException.

The proposed changes look reasonable to me. Those seem to be the only methods 
where removeStaleReferences() could have been indirectly called while looping 
over the entries.

-

Marked as reviewed by dfuchs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20499#pullrequestreview-2227972196

Re: RFR: 8337143: (fc, fs) Move filesystem-related native objects from libnio to libjava [v4]

[Daniel Fuchs]

On Wed, 7 Aug 2024 15:59:14 GMT, Brian Burkhalter  wrote:

>> This proposed change would move the native objects required for NIO file 
>> interaction from the libnio native library to the libjava native library on 
>> Linux, macOS, and Windows.
>
> Brian Burkhalter has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains four additional 
> commits since the last revision:
> 
>  - Merge
>  - 8337143: Removed dependency of libjava on headers in libnio/ch
>  - 8337143: Move natives to /native/libjava/nio/{ch,fs} as a 
> function of their original location in libnio
>  - 8337143: (fc, fs) Move filesystem-related native objects from libnio to 
> libjava

Disclaimer: I have not tried to understand the proposed change in details. 
However I have spotted a small oddity.

src/java.base/share/classes/java/net/Inet6AddressImpl.java line 154:

> 152: static {
> 153: jdk.internal.loader.BootLoader.loadLibrary("net");
> 154: }

I am curious about this change - wouldn't it be needed in Inet4AddressImpl as 
well?

src/java.base/windows/native/libjava/IOUtil.c line 37:

> 35: #include "nio.h"
> 36: #include "nio_util.h"
> 37: /* #include "net_util.h" */

Is this change intended or is this a left over from some experimentation?

-

PR Review: https://git.openjdk.org/jdk/pull/20317#pullrequestreview-2227299000
PR Review Comment: https://git.openjdk.org/jdk/pull/20317#discussion_r1709024031
PR Review Comment: https://git.openjdk.org/jdk/pull/20317#discussion_r1709029492

Re: [jdk23] RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed

[Daniel Fuchs]

On Thu, 20 Jun 2024 15:24:35 GMT, Kevin Walls  wrote:

> 844: JMX attaching of Subject does not work when security manager not 
> allowed

LGTM

-

Marked as reviewed by dfuchs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19810#pullrequestreview-2132226211

Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v19]

[Daniel Fuchs]

On Tue, 18 Jun 2024 12:17:46 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:
> 
>   Additional test runs with SM enabled

The code changes look good to me (if a bit verbose) and the test changes look 
reasonable. It could be beneficial to add some more tests in the future 
involving monitoring and getting the subject from within a monitored MBean.

-

Marked as reviewed by dfuchs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19624#pullrequestreview-2127943873

Re: RFR: 8334490: Normalize string with locale invariant `toLowerCase()`

[Daniel Fuchs]

On Tue, 18 Jun 2024 18:23:12 GMT, Naoto Sato  wrote:

> The test library `jdk.test.lib.Platform` uses no-arg `toLowerCase()` for 
> string comparison, which should be avoided.

+1

-

Marked as reviewed by dfuchs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19775#pullrequestreview-2126251008

Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v16]

[Daniel Fuchs]

On Mon, 17 Jun 2024 20:27:05 GMT, Sean Mullan  wrote:

>> Hi,
>> We almost always check 
>> !SharedSecrets.getJavaLangAccess().allowSecurityManager() (except in the 
>> RMIConnectionImpl contstructor)
>> 
>> This keeps the "modern" case first (except in that constructor, where it is 
>> only one line for each side of the if...).
>> 
>> This extra checking of 
>> SharedSecrets.getJavaLangAccess().allowSecurityManager() is to keep the 
>> modern and old style cases distinct, based on other comments here.  It keeps 
>> it simple to read I hope, but yes it is a little more verbose than it could 
>> be.
>> 
>> I'm hoping you'll agree that as these checks will be removed anyway, 
>> probably with a release, we should delay more extensive cleanup until then.  
>> (I've also  rolled back my noPermissionsACC removal to keep this change away 
>> from being a general cleanup.)
>> 
>> I had a bunch of additional Exception handling for e.g. 
>> PrivilegedActionException I think in here, and it wasn't very pretty.  But, 
>> it might look better when there are fewer nested ifs in these methods, and 
>> when we are properly in the post-SecurityManager world... 8-)
>> 
>> Whether it checks acc != null or acc == null is based on the existing code.  
>> I think that makes this diff easier to read, but also could be reworked and 
>> be made more consistent after SM removal.
>
> Agree with Kevin. To minimize risk, especially since this is to fix a 
> significant regression and we are in RDP1, we are really trying to preserve 
> the existing code as much as possible, even though it could be improved.

It is also more error prone (which is why I suggested using a single well 
tested utility method to implement the temporary hackery rather than spreading 
it at different places in different forms). But I'm OK with the code in this 
form.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1644303322

Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v16]

[Daniel Fuchs]

On Mon, 17 Jun 2024 12:54:44 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 two additional 
> commits since the last revision:
> 
>  - style update
>  - whitespace

The new version looks much better to me. I wonder if we can abstract away some 
of the repeated logic though.

src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
 line 1314:

> 1312: return AccessController.doPrivileged(action, acc);
> 1313: }
> 1314: }

This piece of code is repeated at several places in similar but different forms 
- sometime we check for 
`SharedSecrets.getJavaLangAccess().allowSecurityManager()`, sometime for `! 
SharedSecrets.getJavaLangAccess().allowSecurityManager()`, sometime we check 
for `acc == null`, sometime for `acc != null` etc.. 
I wonder if we could abstract that to some utility method somewhere. Would that 
make sense? Maybe that's not possible due to using sometime `PrivelegedAction` 
and sometimes `PrivilegedExceptionAction` - but maybe we could use 
`PrivilegedExceptionAction` everywhere at the cost of handling 
`PrivilegedActionException`? 
If that doesn't prove useful (if handling `PrivilegedExceptionAction` adds 
again an equivalent amount of clutter) then maybe we could at least make sure 
we do the same checks in the same way (typically `acc == null` vs `acc != 
null`)?

-

PR Review: https://git.openjdk.org/jdk/pull/19624#pullrequestreview-2118725434
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1642864523

Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v13]

[Daniel Fuchs]

On Fri, 14 Jun 2024 15:26:54 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:
> 
>   Unnecessary catches to remove

src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
 line 1437:

> 1435: } catch (Exception e) {
> 1436: if (e instanceof RuntimeException)
> 1437:throw (RuntimeException) e;

Suggestion:

if (e instanceof RuntimeException rte)
   throw rte;

src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
 line 1450:

> 1448: } catch (Exception e) {
> 1449: if (e instanceof RuntimeException)
> 1450:throw (RuntimeException) e;

Suggestion:

if (e instanceof RuntimeException rte)
   throw rte;

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1640033632
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1640034429

Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v6]

[Daniel Fuchs]

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]

[Daniel Fuchs]

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]

[Daniel Fuchs]

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

[Daniel Fuchs]

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]

[Daniel Fuchs]

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]

[Daniel Fuchs]

> 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&pr=19503&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19503&range=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

[Daniel Fuchs]

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&pr=19503&range=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]

[Daniel Fuchs]

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]

[Daniel Fuchs]

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]

[Daniel Fuchs]

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

[Daniel Fuchs]

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

[Daniel Fuchs]

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]

[Daniel Fuchs]

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]

[Daniel Fuchs]

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

[Daniel Fuchs]

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`

[Daniel Fuchs]

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]

[Daniel Fuchs]

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]

[Daniel Fuchs]

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]

[Daniel Fuchs]

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]

[Daniel Fuchs]

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

[Daniel Fuchs]

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]

[Daniel Fuchs]

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]

[Daniel Fuchs]

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

[Daniel Fuchs]

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]

[Daniel Fuchs]

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

[Daniel Fuchs]

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&type=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]

[Daniel Fuchs]

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

[Daniel Fuchs]

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]

[Daniel Fuchs]

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

[Daniel Fuchs]

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]

[Daniel Fuchs]

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]

[Daniel Fuchs]

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]

[Daniel Fuchs]

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]

[Daniel Fuchs]

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

[Daniel Fuchs]

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]

[Daniel Fuchs]

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

[Daniel Fuchs]

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

[Daniel Fuchs]

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

[Daniel Fuchs]

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

[Daniel Fuchs]

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

[Daniel Fuchs]

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

[Daniel Fuchs]

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

[Daniel Fuchs]

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

[Daniel Fuchs]

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

[Daniel Fuchs]

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

[Daniel Fuchs]

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

[Daniel Fuchs]

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

[Daniel Fuchs]

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

[Daniel Fuchs]

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]

[Daniel Fuchs]

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