Re: RFR: 8287766: Unnecessary Vector usage in LdapClient
On Sun, 29 May 2022 20:22:55 GMT, Andrey Turbanov wrote: > In [JDK-8273098](https://bugs.openjdk.java.net/browse/JDK-8273098) I missed > one place, where Vector could be replaced with ArrayList. > Usage of thread-safe collection `Vector` is unnecessary. It's recommended to > use `ArrayList` if a thread-safe implementation is not needed. Looks good - Marked as reviewed by aefimov (Committer). PR: https://git.openjdk.java.net/jdk/pull/8940
Re: RFR: 8287672: jtreg test com/sun/jndi/ldap/LdapPoolTimeoutTest.java fails intermittently in nightly run
On Wed, 1 Jun 2022 15:41:57 GMT, Rob McKenna wrote: > Test change to silently pass if the test environment encounters a > NoRouteToHostException. These are intermittent at the moment but I will > attempt to find an alternative to the use of example.com in some follow up > work. Looks good to me - Marked as reviewed by aefimov (Committer). PR: https://git.openjdk.java.net/jdk/pull/8974
Re: RFR: 8281695: jtreg test com/sun/jndi/ldap/LdapPoolTimeoutTest.java fails intermittently in nightly run
On Fri, 27 May 2022 15:24:46 GMT, Rob McKenna wrote: > Test change to silently pass if the test environment encounters a > NoRouteToHostException. These are intermittent at the moment but I will > attempt to find an alternative to the use of example.com in some follow up > work. The change looks fine, given it is a temporary workaround until an alternative to the use of example.com is found. - Marked as reviewed by aefimov (Committer). PR: https://git.openjdk.java.net/jdk/pull/8925
Re: RFR: 8287544: Replace uses of StringBuffer with StringBuilder in java.naming
On Sun, 29 May 2022 21:57:46 GMT, Andrey Turbanov wrote: > StringBuffer is a legacy synchronized class. StringBuilder is a direct > replacement to StringBuffer which generally have better performance. > There are some code that still uses StringBuffer in java.naming which could > be migrated to `StringBuilder`. > Looks good to me +1 `java.naming` regression tests also happy with the change - no new failures - PR: https://git.openjdk.java.net/jdk/pull/8942
Re: RFR: 8287544: Replace uses of StringBuffer with StringBuilder in java.naming
On Sun, 29 May 2022 21:57:46 GMT, Andrey Turbanov wrote: > StringBuffer is a legacy synchronized class. StringBuilder is a direct > replacement to StringBuffer which generally have better performance. > There are some code that still uses StringBuffer in java.naming which could > be migrated to `StringBuilder`. Looks good to me - Marked as reviewed by aefimov (Committer). PR: https://git.openjdk.java.net/jdk/pull/8942
Re: RFR: 8287497: Use String.contains() instead of String.indexOf() in java.naming
On Sun, 29 May 2022 14:00:20 GMT, Andrey Turbanov wrote: > `String.contains` was introduced in Java 5. > Some code in java.naming still uses old approach with `String.indexOf` to > check if String contains specified substring. > I propose to migrate such usages. Makes code shorter and easier to read. The change looks good to me. I've checked the fix with our CI system and no `java.naming` failures have been discovered. - Marked as reviewed by aefimov (Committer). PR: https://git.openjdk.java.net/jdk/pull/8938
Re: RFR: 8275535: Retrying a failed authentication on multiple LDAP servers can lead to users blocked
On Wed, 20 Oct 2021 13:35:22 GMT, Martin Balao wrote: > I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to > the state previous to JDK-8160768, where an authentication failure stops from > trying other LDAP servers with the same credentials [1]. After JDK-8160768 we > have 2 possible loops to stop: the one that iterates over different URLs and > the one that iterates over different endpoints (after a DNS query that > returns multiple values). > > No test regressions observed in jdk/com/sun/jndi/ldap. > > -- > [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137 Hi @martinuy, I think this fix is in a good state: code changes look good, the CSR is approved and our CI shows no JNDI test failures related to this change. As it was mentioned before the only thing we're waiting for is a bug logged for a test addition with a scenario of how issue can be reproduced. If it is not feasible to do that we can proceed without it - I will log a bug and will use a Spring reproducer shared by Carsten (thank you) as a starting point. Therefore, I'm approving this change. - Marked as reviewed by aefimov (Committer). PR: https://git.openjdk.java.net/jdk/pull/6043
Re: RFR: 8275535: Retrying a failed authentication on multiple LDAP servers can lead to users blocked
On Wed, 9 Feb 2022 19:47:19 GMT, Martin Balao wrote: > Thanks, that was what I initially thought but wanted to check if I was > missing something else given the previous discussion. I should be able to > generate a build for the user and ask him to test in his real environment. As > for the other concern, I'll evaluate options and let you know. Thanks for the update, Martin. I'm ok with pushing the fix without a test given that the user will verify it in his real environment. Maybe, you could also log a bug for a test addition and describe in it an environment, and sort of a high-level scenario of how JDK-8275535 can be reproduced. - PR: https://git.openjdk.java.net/jdk/pull/6043
Re: RFR: 8275535: Retrying a failed authentication on multiple LDAP servers can lead to users blocked
On Wed, 20 Oct 2021 13:35:22 GMT, Martin Balao wrote:
> I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to
> the state previous to JDK-8160768, where an authentication failure stops from
> trying other LDAP servers with the same credentials [1]. After JDK-8160768 we
> have 2 possible loops to stop: the one that iterates over different URLs and
> the one that iterates over different endpoints (after a DNS query that
> returns multiple values).
>
> No test regressions observed in jdk/com/sun/jndi/ldap.
>
> --
> [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137
Hi Martin,
The source changes looks good to me.
In case you have an LDAP server that can be used to reproduce this problem then
maybe you could try to create a test that uses classes from LDAP test library
(`LDAPServer`,`LDAPTestUtils`)?
In a nutshell, it could be done by following these steps:
- Create a regression tests which uses LDAPServer/LDAPTestUtils similar to
tests available in `test/jdk/com/sun/jndi/ldap/blits/AddTests` and
`test/jdk/javax/naming/module/src/test/test`
- Add `-trace` flag (see test/jdk/com/sun/jndi/ldap/lib/LDAPTestUtils.java for
details) as an argument to the test app. When this argument is passed to
LDAPTestUtils.initEnv (see first snippet below) '.ldap' trace file is generated
by a framework. This is where the real LDAP server is required.
- Once `.ldap` trace file is collected use LDAPTestUtils.initEnv and a
ServerSocket to create a test server that will replay the collected trace file
(see second snippet below).
Snippet 1: How trace file can be collected:
/*
* @test
* @library /test/lib ../../lib
* @build LDAPServer LDAPTestUtils /javax/naming/module/src/test/test/
* @run main/othervm TraceExampleTest -trace
*/
public static void collectTrace(String [] args) throws Exception {
Hashtable env;
// initialize test
env = LDAPTestUtils.initEnv(null, "ldap://127.0.0.1:1389;,
TraceExampleTest.class.getName(), args, true);
DirContext ctx = null;
// connect to server
ctx = new InitialDirContext(env);
}
Snippet 2: How trace file can be used (note that the trace file name should
match the test class name in this example) it can be used to create an instance
of the LDAPServer:
/*
* @test
* @library /test/lib ../../lib
* @build LDAPServer LDAPTestUtils /javax/naming/module/src/test/test/
* @run main/othervm TraceExampleTest
*/
public static void runWithTrace(String [] args) throws Exception {
// Create unbound server socket
ServerSocket serverSocket = new ServerSocket();
// Bind it to the loopback address
SocketAddress sockAddr = new InetSocketAddress(
InetAddress.getLoopbackAddress(), 0);
serverSocket.bind(sockAddr);
// Construct the provider URL for LDAPTestUtils
String providerURL = URIBuilder.newBuilder()
.scheme("ldap")
.loopback()
.port(serverSocket.getLocalPort())
.buildUnchecked().toString();
Hashtable env;
// initialize test
env = LDAPTestUtils.initEnv(serverSocket, providerURL,
TraceExampleTest.class.getName(), args, true);
DirContext ctx = null;
// connect to the test LDAP server
ctx = new InitialDirContext(env);
}
-
PR: https://git.openjdk.java.net/jdk/pull/6043
Integrated: 8272996: JNDI DNS provider fails to resolve SRV entries when IPV6 stack is enabled
On Fri, 4 Feb 2022 15:36:35 GMT, Aleksei Efimov wrote: > Hi, > > JNDI's `DnsClient` can fail with `UncheckedIOException` during `connect` or > `disconnect` method calls. It is a [know > behavior](https://bugs.openjdk.java.net/browse/JDK-8236076) of > `DatagramSocket`. > > Currently, `DnsClient` method fast-fails when `UncheckedIOException` is > observed during connect attempt. But it supposed to mark such server as > invalid and continue checking other servers. > > Testing: tier1, tier2, tier3 and existing JCK tests show no failures relevant > to the proposed changed This pull request has now been integrated. Changeset: 4c169495 Author:Aleksei Efimov URL: https://git.openjdk.java.net/jdk/commit/4c169495a2c4bfdcbc82e94e9ca1ee0cc050daf9 Stats: 19 lines in 1 file changed: 11 ins; 6 del; 2 mod 8272996: JNDI DNS provider fails to resolve SRV entries when IPV6 stack is enabled Reviewed-by: dfuchs - PR: https://git.openjdk.java.net/jdk/pull/7353
RFR: 8272996: JNDI DNS provider fails to resolve SRV entries when IPV6 stack is enabled
Hi, JNDI's `DnsClient` can fail with `UncheckedIOException` during `connect` or `disconnect` method calls. It is a [know behavior](https://bugs.openjdk.java.net/browse/JDK-8236076) of `DatagramSocket`. Currently, `DnsClient` method is fast-fails when `UncheckedIOException` is observed during connect attempt. But it supposed to mark such server as invalid and continue checking other servers. Testing: tier1, tier2, tier3 and existing JCK tests show no failures relevant to the proposed changed - Commit messages: - 8272996: JNDI DNS provider fails to resolve SRV entries when IPV6 stack is enabled Changes: https://git.openjdk.java.net/jdk/pull/7353/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7353=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272996 Stats: 19 lines in 1 file changed: 11 ins; 6 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7353.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7353/head:pull/7353 PR: https://git.openjdk.java.net/jdk/pull/7353
Re: RFR: JDK-8277795: ldap connection timeout not honoured under contention [v4]
On Fri, 21 Jan 2022 13:06:40 GMT, Rob McKenna wrote: >> This fix attemps to resolve an issue where threads can stack up on each >> other while waiting to get a connection from the ldap pool to an unreachable >> server. It does this by having each thread start a countdown prior to >> holding the pools' lock. (which has been changed to a ReentrantLock) Once >> the lock has been grabbed, the timeout is adjusted to take the waiting time >> into account and the process of getting a connection from the pool or >> creating a new one commences. >> >> Note: this fix also changes the meaning of the connection pools initSize >> somewhat. In a situation where we have a large initSize and a small timeout >> the first thread could actually exhaust the timeout before creating all of >> its initial connections. Instead this fix simply creates a single connection >> per pool-connection-request. It continues to do so for subsequent requests >> regardless of whether an existing unused connection is available in the pool >> until initSize is exhausted. As such it may require a CSR. > > Rob McKenna 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 seven additional > commits since the last revision: > > - Add method to guard long to int cast > - Allow the test to pass on MacOSX > - JDK-8277795: whitespace > - Add test > - JDK-8277795: formatting > - JDK-8277795: whitespace cleanup > - JDK-8277795: ldap connection timeout not honoured under contention Marked as reviewed by aefimov (Committer). - PR: https://git.openjdk.java.net/jdk/pull/6568
Re: RFR: 8278892: java.naming module description is missing @uses tags to document the services that it uses
On Wed, 12 Jan 2022 05:48:26 GMT, Masanori Yano wrote: > I have fixed the javadoc comments as the definition. Could you review this > fix? Hi @masyano, Thanks for fixing `java.naming` module javadoc. The fix looks fine to me. - Marked as reviewed by aefimov (Committer). PR: https://git.openjdk.java.net/jdk/pull/7041
Re: RFR: JDK-8277795: ldap connection timeout not honoured under contention [v3]
On Tue, 14 Dec 2021 15:26:54 GMT, Rob McKenna wrote: >> This fix attemps to resolve an issue where threads can stack up on each >> other while waiting to get a connection from the ldap pool to an unreachable >> server. It does this by having each thread start a countdown prior to >> holding the pools' lock. (which has been changed to a ReentrantLock) Once >> the lock has been grabbed, the timeout is adjusted to take the waiting time >> into account and the process of getting a connection from the pool or >> creating a new one commences. >> >> Note: this fix also changes the meaning of the connection pools initSize >> somewhat. In a situation where we have a large initSize and a small timeout >> the first thread could actually exhaust the timeout before creating all of >> its initial connections. Instead this fix simply creates a single connection >> per pool-connection-request. It continues to do so for subsequent requests >> regardless of whether an existing unused connection is available in the pool >> until initSize is exhausted. As such it may require a CSR. > > Rob McKenna has updated the pull request incrementally with one additional > commit since the last revision: > > Allow the test to pass on MacOSX The changes look good to me. Few minor comments: The last modification year in copyright headers can be updated. test/jdk/com/sun/jndi/ldap/LdapPoolTimeoutTest.java line 29: > 27: * lib/ > 28: * @run testng/othervm LdapPoolTimeoutTest > 29: * @bug JDK-8277795 I believe the `JDK-` prefix is not needed in `@ bug` tag value. Tags can be also reordered to follow the recommendations [here](https://openjdk.java.net/jtreg/tag-spec.html#ORDER). - PR: https://git.openjdk.java.net/jdk/pull/6568
Integrated: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI
On Tue, 5 Oct 2021 12:00:15 GMT, Aleksei Efimov wrote: > This change implements a new service provider interface for host name and > address resolution, so that java.net.InetAddress API can make use of > resolvers other than the platform's built-in resolver. > > The following API classes are added to `java.net.spi` package to facilitate > this: > - `InetAddressResolverProvider` - abstract class defining a service, and is, > essentially, a factory for `InetAddressResolver` resolvers. > - `InetAddressResolverProvider.Configuration ` - an interface describing the > platform's built-in configuration for resolution operations that could be > used to bootstrap a resolver construction, or to implement partial delegation > of lookup operations. > - `InetAddressResolver` - an interface that defines methods for the > fundamental forward and reverse lookup operations. > - `InetAddressResolver.LookupPolicy` - a class whose instances describe the > characteristics of one forward lookup operation. > > More details in [JEP-418](https://openjdk.java.net/jeps/418). > > Testing: new and existing `tier1:tier3` tests This pull request has now been integrated. Changeset: 2ca4ff87 Author:Aleksei Efimov URL: https://git.openjdk.java.net/jdk/commit/2ca4ff87b7c31d56542bbdcea70e828be33f4e97 Stats: 3141 lines in 56 files changed: 2848 ins; 155 del; 138 mod 8244202: Implementation of JEP 418: Internet-Address Resolution SPI Co-authored-by: Chris Hegarty Co-authored-by: Daniel Fuchs Co-authored-by: Alan Bateman Reviewed-by: dfuchs, alanb, michaelm, chegar - PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v13]
> This change implements a new service provider interface for host name and > address resolution, so that java.net.InetAddress API can make use of > resolvers other than the platform's built-in resolver. > > The following API classes are added to `java.net.spi` package to facilitate > this: > - `InetAddressResolverProvider` - abstract class defining a service, and is, > essentially, a factory for `InetAddressResolver` resolvers. > - `InetAddressResolverProvider.Configuration ` - an interface describing the > platform's built-in configuration for resolution operations that could be > used to bootstrap a resolver construction, or to implement partial delegation > of lookup operations. > - `InetAddressResolver` - an interface that defines methods for the > fundamental forward and reverse lookup operations. > - `InetAddressResolver.LookupPolicy` - a class whose instances describe the > characteristics of one forward lookup operation. > > More details in [JEP-418](https://openjdk.java.net/jeps/418). > > Testing: new and existing `tier1:tier3` tests Aleksei Efimov has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 19 commits: - Merge branch 'master' into JDK-8244202_JEP418_impl - Merge branch 'master' into JDK-8244202_JEP418_impl - Merge branch 'master' into JDK-8244202_JEP418_impl - Replace 'system' with 'the system-wide', move comment section - Add @ throws NPE to hosts file resolver javadoc - Changes to address review comments - Update tests to address SM deprecation - Merge branch 'master' into JDK-8244202_JEP418_impl - More javadoc updates to API classes - Review updates + move resolver docs to the provider class (CSR update to follow) - ... and 9 more: https://git.openjdk.java.net/jdk/compare/f561d3c1...a2498d2b - Changes: https://git.openjdk.java.net/jdk/pull/5822/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5822=12 Stats: 3141 lines in 56 files changed: 2848 ins; 155 del; 138 mod Patch: https://git.openjdk.java.net/jdk/pull/5822.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5822/head:pull/5822 PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8275535: Retrying a failed authentication on multiple LDAP servers can lead to users blocked
On Wed, 20 Oct 2021 13:35:22 GMT, Martin Balao wrote: > I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to > the state previous to JDK-8160768, where an authentication failure stops from > trying other LDAP servers with the same credentials [1]. After JDK-8160768 we > have 2 possible loops to stop: the one that iterates over different URLs and > the one that iterates over different endpoints (after a DNS query that > returns multiple values). > > No test regressions observed in jdk/com/sun/jndi/ldap. > > -- > [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137 Hi Martin, The change looks reasonable to me. I would suggest having a CSR logged for this change due to the following [behavioral incompatibility](https://wiki.openjdk.java.net/display/csr/Kinds+of+Compatibility): Before the change - all available endpoints/URLs are tried to create an LDAP context. With the proposed change - incorrect credentials will prevent other endpoints to be exercised to create an LDAP context. Having a CSR will also help to document difference in handling `AuthenticationException` and `NamingException` during construction of an LDAP context from the list of endpoints acquired from a LDAP DNS provider. - PR: https://git.openjdk.java.net/jdk/pull/6043
Re: Fix AbstractLdapNamingEnumeration next to throw NoSuchElementException instead of NullPointerException
Hi Andrew, I've logged a JBS bug for your change: https://bugs.openjdk.java.net/browse/JDK-8276939 You can use it to update your PR title to address the following jcheck error: The commit message does not reference any issue. To add an issue reference to this PR, edit the title to be of the format issue number: message. With Best Regards, Aleksei From: core-libs-dev on behalf of Andrew Luo Sent: Sunday, October 24, 2021 9:47 PM To: core-libs-dev Subject: Fix AbstractLdapNamingEnumeration next to throw NoSuchElementException instead of NullPointerException I have a small PR to fix a bug in AbstractLdapNamingEnumeration - details are in the PR: https://github.com/openjdk/jdk/pull/6095 (Note: I'm already an OpenJDK contributor and have signed an OCA but don't have an author account to be able to create bugs) Thanks, -Andrew
Re: RFR: 8276629: Use blessed modifier order in core-libs code
On Thu, 4 Nov 2021 11:31:50 GMT, Pavel Rappo wrote: > I can see that this PR changes java.naming. Another bug to change > java.naming, JDK-8276552, was filed yesterday. Please check with its reporter > and coordinate this effort if necessary. @magicus I'm ok with having `java.naming` changes integrated as part of this PR. I'll close `JDK-8276552` as duplicate. The changes looks fine to me. - PR: https://git.openjdk.java.net/jdk/pull/6250
Re: RFR: 8276629: Use blessed modifier order in core-libs code
On Thu, 4 Nov 2021 11:09:42 GMT, Magnus Ihse Bursie wrote: > I ran bin/blessed-modifier-order.sh on source owned by core-libs. This > scripts verifies that modifiers are in the "blessed" order, and fixes it > otherwise. I have manually checked the changes made by the script to make > sure they are sound. Marked as reviewed by aefimov (Committer). - PR: https://git.openjdk.java.net/jdk/pull/6250
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v12]
> This change implements a new service provider interface for host name and > address resolution, so that java.net.InetAddress API can make use of > resolvers other than the platform's built-in resolver. > > The following API classes are added to `java.net.spi` package to facilitate > this: > - `InetAddressResolverProvider` - abstract class defining a service, and is, > essentially, a factory for `InetAddressResolver` resolvers. > - `InetAddressResolverProvider.Configuration ` - an interface describing the > platform's built-in configuration for resolution operations that could be > used to bootstrap a resolver construction, or to implement partial delegation > of lookup operations. > - `InetAddressResolver` - an interface that defines methods for the > fundamental forward and reverse lookup operations. > - `InetAddressResolver.LookupPolicy` - a class whose instances describe the > characteristics of one forward lookup operation. > > More details in [JEP-418](https://openjdk.java.net/jeps/418). > > Testing: new and existing `tier1:tier3` tests Aleksei Efimov has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 18 commits: - Merge branch 'master' into JDK-8244202_JEP418_impl - Merge branch 'master' into JDK-8244202_JEP418_impl - Replace 'system' with 'the system-wide', move comment section - Add @ throws NPE to hosts file resolver javadoc - Changes to address review comments - Update tests to address SM deprecation - Merge branch 'master' into JDK-8244202_JEP418_impl - More javadoc updates to API classes - Review updates + move resolver docs to the provider class (CSR update to follow) - Change InetAddressResolver method names - ... and 8 more: https://git.openjdk.java.net/jdk/compare/a316c06e...0542df51 - Changes: https://git.openjdk.java.net/jdk/pull/5822/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5822=11 Stats: 3141 lines in 56 files changed: 2848 ins; 155 del; 138 mod Patch: https://git.openjdk.java.net/jdk/pull/5822.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5822/head:pull/5822 PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v11]
> This change implements a new service provider interface for host name and > address resolution, so that java.net.InetAddress API can make use of > resolvers other than the platform's built-in resolver. > > The following API classes are added to `java.net.spi` package to facilitate > this: > - `InetAddressResolverProvider` - abstract class defining a service, and is, > essentially, a factory for `InetAddressResolver` resolvers. > - `InetAddressResolverProvider.Configuration ` - an interface describing the > platform's built-in configuration for resolution operations that could be > used to bootstrap a resolver construction, or to implement partial delegation > of lookup operations. > - `InetAddressResolver` - an interface that defines methods for the > fundamental forward and reverse lookup operations. > - `InetAddressResolver.LookupPolicy` - a class whose instances describe the > characteristics of one forward lookup operation. > > More details in [JEP-418](https://openjdk.java.net/jeps/418). > > Testing: new and existing `tier1:tier3` tests 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 17 additional commits since the last revision: - Merge branch 'master' into JDK-8244202_JEP418_impl - Replace 'system' with 'the system-wide', move comment section - Add @ throws NPE to hosts file resolver javadoc - Changes to address review comments - Update tests to address SM deprecation - Merge branch 'master' into JDK-8244202_JEP418_impl - More javadoc updates to API classes - Review updates + move resolver docs to the provider class (CSR update to follow) - Change InetAddressResolver method names - Remove no longer used import from IPSupport - ... and 7 more: https://git.openjdk.java.net/jdk/compare/9152e3fb...b31d61d1 - Changes: - all: https://git.openjdk.java.net/jdk/pull/5822/files - new: https://git.openjdk.java.net/jdk/pull/5822/files/f660cc6e..b31d61d1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5822=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5822=09-10 Stats: 16073 lines in 487 files changed: 12211 ins; 1991 del; 1871 mod Patch: https://git.openjdk.java.net/jdk/pull/5822.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5822/head:pull/5822 PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v10]
> This change implements a new service provider interface for host name and > address resolution, so that java.net.InetAddress API can make use of > resolvers other than the platform's built-in resolver. > > The following API classes are added to `java.net.spi` package to facilitate > this: > - `InetAddressResolverProvider` - abstract class defining a service, and is, > essentially, a factory for `InetAddressResolver` resolvers. > - `InetAddressResolverProvider.Configuration ` - an interface describing the > platform's built-in configuration for resolution operations that could be > used to bootstrap a resolver construction, or to implement partial delegation > of lookup operations. > - `InetAddressResolver` - an interface that defines methods for the > fundamental forward and reverse lookup operations. > - `InetAddressResolver.LookupPolicy` - a class whose instances describe the > characteristics of one forward lookup operation. > > More details in [JEP-418](https://openjdk.java.net/jeps/418). > > Testing: new and existing `tier1:tier3` tests Aleksei Efimov has updated the pull request incrementally with one additional commit since the last revision: Replace 'system' with 'the system-wide', move comment section - Changes: - all: https://git.openjdk.java.net/jdk/pull/5822/files - new: https://git.openjdk.java.net/jdk/pull/5822/files/2ca78ba9..f660cc6e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5822=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5822=08-09 Stats: 8 lines in 1 file changed: 4 ins; 3 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5822.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5822/head:pull/5822 PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v9]
On Wed, 27 Oct 2021 16:23:29 GMT, Michael McMahon wrote:
>> Aleksei Efimov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add @ throws NPE to hosts file resolver javadoc
>
> src/java.base/share/classes/java/net/InetAddress.java line 841:
>
>> 839: // 'resolver.lookupByAddress' and 'InetAddress.getAllByName0'
>> delegate to the system-wide resolver,
>> 840: // which could be a custom one. At that point we treat any
>> unexpected RuntimeException thrown by
>> 841: // the resolver as we would treat an UnknownHostException or an
>> unmatched host name.
>
> indentation issue in comment above
Thanks - moved the comment block inside `catch` block
(f660cc6ecc7a31c83de220160b9fd8d0fbacd1be)
> src/java.base/share/classes/java/net/InetAddress.java line 1308:
>
>> 1306: * Creates an InetAddress based on the provided host name and IP
>> address.
>> 1307: * System {@linkplain InetAddressResolver resolver} is not used to
>> check
>> 1308: * the validity of the address.
>
> Is this term "system resolver" defined somewhere? I think it means the
> configured resolver for the current VM. Previously, it really was the system
> resolver. So, I think it's potentially confusing, as there is also reference
> to the java.net.preferIPv6Addresses system property as having a possible
> value "system" which refers to the operating system. Since the CSR is
> approved, I'm happy to discuss this point post integration.
Thanks for highlighting that: Changed `"system"` to `"the system-wide"` -
that's what was originally meant by `"system resolver"`
(f660cc6ecc7a31c83de220160b9fd8d0fbacd1be).
-
PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8276042: Remove unused local variables in java.naming [v2]
On Wed, 27 Oct 2021 15:42:32 GMT, Andrey Turbanov wrote: >> 8276042: Remove unused local variables in java.naming > > Andrey Turbanov has updated the pull request incrementally with one > additional commit since the last revision: > > 8276042: Remove unused local variables in java.naming > use instanceof pattern Marked as reviewed by aefimov (Committer). - PR: https://git.openjdk.java.net/jdk/pull/6091
Re: RFR: 8276042: Remove unused local variables in java.naming
On Sat, 23 Oct 2021 12:51:15 GMT, Andrey Turbanov wrote:
> 8276042: Remove unused local variables in java.naming
Hi Andrey,
Thanks for cleaning up the code.
Changes look good to me, with one suggestion below.
src/java.naming/share/classes/com/sun/jndi/toolkit/ctx/PartialCompositeContext.java
line 514:
> 512: Object obj = cont.getResolvedObj();
> 513:
> 514: if (obj instanceof PartialCompositeContext) {
Since we're changing this method, maybe `instanceof` pattern matching can be
used here to remove casting below.
-
Marked as reviewed by aefimov (Committer).
PR: https://git.openjdk.java.net/jdk/pull/6091
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v8]
On Tue, 26 Oct 2021 15:04:54 GMT, Daniel Fuchs wrote: >> 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 14 additional >> commits since the last revision: >> >> - Changes to address review comments >> - Update tests to address SM deprecation >> - Merge branch 'master' into JDK-8244202_JEP418_impl >> - More javadoc updates to API classes >> - Review updates + move resolver docs to the provider class (CSR update to >> follow) >> - Change InetAddressResolver method names >> - Remove no longer used import from IPSupport >> - Rename IPSupport.hasAddress and update it to use SocketChannel.open >> - Address review comments: javadoc + code cleanup >> - Address resolver bootstraping issue >> - ... and 4 more: >> https://git.openjdk.java.net/jdk/compare/1bbecc93...1378686b > > src/java.base/share/classes/java/net/InetAddress.java line 1151: > >> 1149: >> 1150: Objects.requireNonNull(host); >> 1151: Objects.requireNonNull(lookupPolicy); > > for consistency we could add `@throws NullPointerException` to the API doc of > that method - since it seems to have everything else... Added `@throws NullPointerException` to the hosts file resolver API docs: 2ca78ba98dc81cd6cc44b53dc7d56a6ae42c2736 - PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v9]
> This change implements a new service provider interface for host name and > address resolution, so that java.net.InetAddress API can make use of > resolvers other than the platform's built-in resolver. > > The following API classes are added to `java.net.spi` package to facilitate > this: > - `InetAddressResolverProvider` - abstract class defining a service, and is, > essentially, a factory for `InetAddressResolver` resolvers. > - `InetAddressResolverProvider.Configuration ` - an interface describing the > platform's built-in configuration for resolution operations that could be > used to bootstrap a resolver construction, or to implement partial delegation > of lookup operations. > - `InetAddressResolver` - an interface that defines methods for the > fundamental forward and reverse lookup operations. > - `InetAddressResolver.LookupPolicy` - a class whose instances describe the > characteristics of one forward lookup operation. > > More details in [JEP-418](https://openjdk.java.net/jeps/418). > > Testing: new and existing `tier1:tier3` tests Aleksei Efimov has updated the pull request incrementally with one additional commit since the last revision: Add @ throws NPE to hosts file resolver javadoc - Changes: - all: https://git.openjdk.java.net/jdk/pull/5822/files - new: https://git.openjdk.java.net/jdk/pull/5822/files/1378686b..2ca78ba9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5822=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5822=07-08 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5822.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5822/head:pull/5822 PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v5]
On Sat, 23 Oct 2021 06:33:52 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/java/net/spi/InetAddressResolverProvider.java
>> line 52:
>>
>>> 50: /**
>>> 51: * Initialise and return the {@link InetAddressResolver} provided by
>>> 52: * this provider. This method is called by {@link InetAddress} when
>>
>> "the InetAddressResolver" suggests there is just one instance. That is
>> probably the case but I don't think you want to be restricted to that.
>
> Initialise -> Initialize to be consistent with other usages that use US
> spelling.
Thanks, changed in 1378686becfcbf18793556de8381b5ebcd79698d
-
PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v7]
On Tue, 26 Oct 2021 12:49:30 GMT, Aleksei Efimov wrote:
>> src/java.base/share/classes/java/net/spi/InetAddressResolverProvider.java
>> line 45:
>>
>>> 43: * system-wide resolver instance, which is used by
>>> 44: * >> href="{@docRoot}/java.base/java/net/InetAddress.html#host-name-resolution">
>>> 45: * InetAddress.
>>
>> I think we should prune some some of the text from the class description to
>> avoid repetition. Here's a suggestion:
>>
>> 1. Add the following immediately after the sentence "A given innovation ..."
>> "It is set after the VM is fully initialized and when an invocation of a
>> method in InetAddress class triggers the first lookup operation.
>>
>> 2. Replace the text in L47-65 with:
>> "A resolver provider is located and loaded by InetAddress to create the
>> systwm-wide resolver as follows:
>>
>> 3. Replace the remaining three usages of "install" with "set".
>
> Thanks, changed as suggested: 1378686becfcbf18793556de8381b5ebcd79698d
Pruned `InetAddressResolverProvider` class-level doc as suggested:
1378686becfcbf18793556de8381b5ebcd79698d
-
PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v7]
On Sat, 23 Oct 2021 06:19:46 GMT, Alan Bateman wrote:
>> Aleksei Efimov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> More javadoc updates to API classes
>
> src/java.base/share/classes/java/net/InetAddress.java line 169:
>
>> 167: * Access Protocol (LDAP).
>> 168: * The particular naming services that the built-in resolver uses by
>> default
>> 169: * depend on the configuration of the local machine.
>
> depend -> depends
Thanks, changed in 1378686becfcbf18793556de8381b5ebcd79698d
> src/java.base/share/classes/java/net/spi/InetAddressResolver.java line 38:
>
>> 36: * deployed as a > href="InetAddressResolverProvider.html#system-wide-resolver">
>> 37: * system-wide resolver. {@link InetAddress} delegates all lookup
>> requests to
>> 38: * the deployed system-wide resolver instance.
>
> I think the class description would be a bit clearer if we drop sentence 2
> because it overlaps with paragraph 2. I think we can adjust sentence 3 to
> "InetAddress delegates all lookup operations to the system-wide resolver" and
> it will all flow nicely.
Thanks, changed as suggested in 1378686becfcbf18793556de8381b5ebcd79698d
> src/java.base/share/classes/java/net/spi/InetAddressResolver.java line 88:
>
>> 86: * to a valid IP address length
>> 87: */
>> 88: String lookupByAddress(byte[] addr) throws UnknownHostException;
>
> I assume this throws NPE if addr is null.
1378686becfcbf18793556de8381b5ebcd79698d: added @ throws NPE javadoc, also
added additional checks for `null` parameter values into the built-in and hosts
file resolver implementations.
> src/java.base/share/classes/java/net/spi/InetAddressResolverProvider.java
> line 45:
>
>> 43: * system-wide resolver instance, which is used by
>> 44: * > href="{@docRoot}/java.base/java/net/InetAddress.html#host-name-resolution">
>> 45: * InetAddress.
>
> I think we should prune some some of the text from the class description to
> avoid repetition. Here's a suggestion:
>
> 1. Add the following immediately after the sentence "A given innovation ..."
> "It is set after the VM is fully initialized and when an invocation of a
> method in InetAddress class triggers the first lookup operation.
>
> 2. Replace the text in L47-65 with:
> "A resolver provider is located and loaded by InetAddress to create the
> systwm-wide resolver as follows:
>
> 3. Replace the remaining three usages of "install" with "set".
Thanks, changed as suggested: 1378686becfcbf18793556de8381b5ebcd79698d
-
PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v8]
> This change implements a new service provider interface for host name and > address resolution, so that java.net.InetAddress API can make use of > resolvers other than the platform's built-in resolver. > > The following API classes are added to `java.net.spi` package to facilitate > this: > - `InetAddressResolverProvider` - abstract class defining a service, and is, > essentially, a factory for `InetAddressResolver` resolvers. > - `InetAddressResolverProvider.Configuration ` - an interface describing the > platform's built-in configuration for resolution operations that could be > used to bootstrap a resolver construction, or to implement partial delegation > of lookup operations. > - `InetAddressResolver` - an interface that defines methods for the > fundamental forward and reverse lookup operations. > - `InetAddressResolver.LookupPolicy` - a class whose instances describe the > characteristics of one forward lookup operation. > > More details in [JEP-418](https://openjdk.java.net/jeps/418). > > Testing: new and existing `tier1:tier3` tests 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 14 additional commits since the last revision: - Changes to address review comments - Update tests to address SM deprecation - Merge branch 'master' into JDK-8244202_JEP418_impl - More javadoc updates to API classes - Review updates + move resolver docs to the provider class (CSR update to follow) - Change InetAddressResolver method names - Remove no longer used import from IPSupport - Rename IPSupport.hasAddress and update it to use SocketChannel.open - Address review comments: javadoc + code cleanup - Address resolver bootstraping issue - ... and 4 more: https://git.openjdk.java.net/jdk/compare/a8495826...1378686b - Changes: - all: https://git.openjdk.java.net/jdk/pull/5822/files - new: https://git.openjdk.java.net/jdk/pull/5822/files/fa655be2..1378686b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5822=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5822=06-07 Stats: 32082 lines in 838 files changed: 22928 ins; 6060 del; 3094 mod Patch: https://git.openjdk.java.net/jdk/pull/5822.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5822/head:pull/5822 PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v5]
On Wed, 20 Oct 2021 15:41:35 GMT, Daniel Fuchs wrote: >> Aleksei Efimov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Change InetAddressResolver method names > > Marked as reviewed by dfuchs (Reviewer). @dfuch @AlanBateman fa655be2bb0a402b70916567d34cc29a7898f938 and 2a554c91864e3b42a0ea315b0a671782fe341927 contain reworked `InetAddress`/`InetAddressResolverProvider`/`InetAddressResolver` specs with the following changes: - `InetAddress Resolver Providers` InetAddress section was modified and moved to `InetAddressResolverProvider`. - `Host Name Resolution` InetAddress section was updated to reference new InetAddress resolver SPI. - Changes for previous review comments. - javadoc formatting clean-up - PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v7]
> This change implements a new service provider interface for host name and > address resolution, so that java.net.InetAddress API can make use of > resolvers other than the platform's built-in resolver. > > The following API classes are added to `java.net.spi` package to facilitate > this: > - `InetAddressResolverProvider` - abstract class defining a service, and is, > essentially, a factory for `InetAddressResolver` resolvers. > - `InetAddressResolverProvider.Configuration ` - an interface describing the > platform's built-in configuration for resolution operations that could be > used to bootstrap a resolver construction, or to implement partial delegation > of lookup operations. > - `InetAddressResolver` - an interface that defines methods for the > fundamental forward and reverse lookup operations. > - `InetAddressResolver.LookupPolicy` - a class whose instances describe the > characteristics of one forward lookup operation. > > More details in [JEP-418](https://openjdk.java.net/jeps/418). > > Testing: new and existing `tier1:tier3` tests Aleksei Efimov has updated the pull request incrementally with one additional commit since the last revision: More javadoc updates to API classes - Changes: - all: https://git.openjdk.java.net/jdk/pull/5822/files - new: https://git.openjdk.java.net/jdk/pull/5822/files/2a554c91..fa655be2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5822=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5822=05-06 Stats: 88 lines in 3 files changed: 22 ins; 8 del; 58 mod Patch: https://git.openjdk.java.net/jdk/pull/5822.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5822/head:pull/5822 PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v6]
> This change implements a new service provider interface for host name and > address resolution, so that java.net.InetAddress API can make use of > resolvers other than the platform's built-in resolver. > > The following API classes are added to `java.net.spi` package to facilitate > this: > - `InetAddressResolverProvider` - abstract class defining a service, and is, > essentially, a factory for `InetAddressResolver` resolvers. > - `InetAddressResolverProvider.Configuration ` - an interface describing the > platform's built-in configuration for resolution operations that could be > used to bootstrap a resolver construction, or to implement partial delegation > of lookup operations. > - `InetAddressResolver` - an interface that defines methods for the > fundamental forward and reverse lookup operations. > - `InetAddressResolver.LookupPolicy` - a class whose instances describe the > characteristics of one forward lookup operation. > > More details in [JEP-418](https://openjdk.java.net/jeps/418). > > Testing: new and existing `tier1:tier3` tests Aleksei Efimov has updated the pull request incrementally with one additional commit since the last revision: Review updates + move resolver docs to the provider class (CSR update to follow) - Changes: - all: https://git.openjdk.java.net/jdk/pull/5822/files - new: https://git.openjdk.java.net/jdk/pull/5822/files/30226481..2a554c91 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5822=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5822=04-05 Stats: 123 lines in 3 files changed: 49 ins; 40 del; 34 mod Patch: https://git.openjdk.java.net/jdk/pull/5822.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5822/head:pull/5822 PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v5]
On Wed, 20 Oct 2021 18:47:32 GMT, Alan Bateman wrote: >> Aleksei Efimov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Change InetAddressResolver method names > > src/java.base/share/classes/java/net/InetAddress.java line 244: > >> 242: * @implNote >> 243: * For any lookup operation that might occur before the VM is fully >> booted the built-in >> 244: * resolver will be used. > > I think we will need decide if InetAddress class description is the right > place for this or whether some/most of it should move to > InetAddressResolverProvider. It might be that we update the existing "Host > Name Resolution" section with some basic/readable text to make the reader > aware that there is a provider mechanism with a link to > InetAddressResolverProvider with the details. Thanks for a good suggestion, Alan. It looks better now (2a554c91864e3b42a0ea315b0a671782fe341927) with some basic text for provider mechanism added to `Host Name Resolution`, and `InetAddress Resolver Providers` section moved to `InetAddressResolverProvider`. - PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v3]
On Sun, 17 Oct 2021 21:03:56 GMT, Mark Sheppard wrote: > getByName requires a hostname lookup and getByAdress requires (eventually - I > know the docs says there’s no reverse lookup) an address reverse lookup. > Thus, a logical mapping is getByName —> lookupHostname, and getByAddr —> > lookupAddress, but the opposite will occur getByName --> lookupAddresses and > getByAddress --> lookupHostname. Therfore I proffer maps neatly to rename > methods to resolveHostname and resolveAddress such that the mapping are > getByName --> resolveHostname and getByAddress --> resolveAddress. Thank you, Mark, for highlighting the naming contradiction in the `InetAddressResolver` interface method names. It can be solved by making these names symmetrical to `InetAddress` API. In 302264810725f86a4569161754f7b052c78fd826 `InetAddressResolver` method names have been updated to stress input parameters type, which brings them in sync with `InetAddress` API: - `InetAddressResolver.lookupAddresses` is renamed to `InetAddressResolver.lookupByName` - `InetAddressResolver.lookupHostName` is renamed to `InetAddressResolver.lookupByAddress` - PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v5]
> This change implements a new service provider interface for host name and > address resolution, so that java.net.InetAddress API can make use of > resolvers other than the platform's built-in resolver. > > The following API classes are added to `java.net.spi` package to facilitate > this: > - `InetAddressResolverProvider` - abstract class defining a service, and is, > essentially, a factory for `InetAddressResolver` resolvers. > - `InetAddressResolverProvider.Configuration ` - an interface describing the > platform's built-in configuration for resolution operations that could be > used to bootstrap a resolver construction, or to implement partial delegation > of lookup operations. > - `InetAddressResolver` - an interface that defines methods for the > fundamental forward and reverse lookup operations. > - `InetAddressResolver.LookupPolicy` - a class whose instances describe the > characteristics of one forward lookup operation. > > More details in [JEP-418](https://openjdk.java.net/jeps/418). > > Testing: new and existing `tier1:tier3` tests Aleksei Efimov has updated the pull request incrementally with one additional commit since the last revision: Change InetAddressResolver method names - Changes: - all: https://git.openjdk.java.net/jdk/pull/5822/files - new: https://git.openjdk.java.net/jdk/pull/5822/files/ac0d2184..30226481 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5822=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5822=03-04 Stats: 33 lines in 9 files changed: 0 ins; 0 del; 33 mod Patch: https://git.openjdk.java.net/jdk/pull/5822.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5822/head:pull/5822 PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v3]
On Sun, 17 Oct 2021 21:39:06 GMT, Mark Sheppard wrote: > I think that a hostname is constant while a host is up, but it can be > changed, and when changed a host restart is required. I don't think it is > quite as dynamic as has been suggested, but I open to correction. It is possible to change a host name without host restart. What has been tested: - Checked O/S type - Linux - `hostnamectl` cmd was used to change a host name - In output below there is only one `jshell` instance running. jshell> InetAddress.getLocalHost() $1 ==> test-host-name/192.168.0.155 $ hostnamectl set-hostname 'test-host-name-changed' # Need to sleep for 5 seconds since local host # is cached for 5 seconds since last resolution $ sleep 5 jshell> InetAddress.getLocalHost() $2 ==> test-host-name-changed/192.168.0.155 I believe it proves that we need to treat a host name as dynamically changing information. - PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v3]
On Fri, 15 Oct 2021 17:19:26 GMT, Daniel Fuchs wrote:
>> Aleksei Efimov has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Add @since tags to new API classes
>> - Add checks and test for empty stream resolver results
>
> test/lib/jdk/test/lib/net/IPSupport.java line 88:
>
>> 86: } catch (SocketException se2) {
>> 87: return false;
>> 88: }
>
> The implementation of this method now conflicts with its name. Maybe we
> should pass a `Class` as parameter, and construct the
> loopback address from there. IIRC the issue with the original implementation
> was that it would say that IPv6 was not supported if IPv6 had only been
> disabled on the loopback interface - and that caused trouble because the
> native implementation of the built-in resolver had a different view of the
> situation - and saw that an IPv6 stack was available on the machine. Maybe
> this would deserve a comment too - so that future maintainers can figure out
> what we are trying to do here.
Thanks for a good suggestion: renamed to `isSupported`, changed parameter type
to `Class addressType` and updated it to use
`SocketChannel.open(ProtocolFamily pf)`
[API](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/nio/channels/SocketChannel.html#open(java.net.ProtocolFamily))
added in `15` to check if the plafrom supports addresses of specified address
type. Pushed as 95a21e57fbe8be147d23e6a6901ae307e8237cbb.
All new tests (especially `LookupPolicyMappingTest`) pass in environment with
`IPv6` addresses disabled.
-
PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v3]
On Fri, 15 Oct 2021 17:09:46 GMT, Daniel Fuchs wrote:
>> Aleksei Efimov has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Add @since tags to new API classes
>> - Add checks and test for empty stream resolver results
>
> test/jdk/java/net/spi/InetAddressResolverProvider/providers/recursive/recursive.init.provider/impl/InetAddressUsageInGetProviderImpl.java
> line 38:
>
>> 36: String localHostName;
>> 37: try {
>> 38: localHostName = InetAddress.getLocalHost().getHostName();
>
> Try to call InetAddress.getLocalHost().getHostName(); twice here.
New `BootstrapResolverUsageTest` test is added to trigger the bootstrap
resolver problem. `InetAddress.getByName` calls with unique names are used
instead of `InetAddress.getLocalHost()` to avoid caching of LHN resolution
results. Added in cd19ecd63838d3c2b713ed66d41107fe01a2d3e6.
-
PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v3]
On Fri, 15 Oct 2021 14:25:02 GMT, Daniel Fuchs wrote:
>> Aleksei Efimov has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Add @since tags to new API classes
>> - Add checks and test for empty stream resolver results
>
> src/java.base/share/classes/java/net/InetAddress.java line 231:
>
>> 229: * The first provider found will be used to instantiate the {@link
>> InetAddressResolver InetAddressResolver}
>> 230: * by invoking the {@link
>> InetAddressResolverProvider#get(InetAddressResolverProvider.Configuration)}
>> 231: * method. The instantiated {@code InetAddressResolver} will be
>> installed as the system-wide
>
> Maybe "... method. The **returned** {@code InetAddressResolver} will be
> installed ..."
Changed as suggested in 648e65b8e6ae9687f7164ea3e81c51ab2b5e2dfe.
> src/java.base/share/classes/java/net/InetAddress.java line 486:
>
>> 484: return cns;
>> 485: } finally {
>> 486: bootstrapResolver = null;
>
> This is incorrect. This will set bootstrapResolver to null in the case where
> you return it at line 468 - which means that a second call will then find it
> null. I believe you want to set it to null in the finally clause only if you
> reached line 470. You could achieve this by declaring a local variable - e.g
> `InetAddressResolver tempResolver = null;` before entering the try-finally
> then set that variable at line 470 `return tempResolver = bootstrapResolver;`
> and in finally do `if (tempResolver == null) bootsrapResolver = null;`
>
> To test this you could try to call e.g. `InetAddress.getByName` twice in
> succession in your test: I believe it will fail with the code as it stands,
> but will succeed with my proposed changes...
Good catch, thank you Daniel. Added a test for that and fixed it in
cd19ecd63838d3c2b713ed66d41107fe01a2d3e6. Now the `bootstrapResolver` field is
set back to `null` in finally block only if a call to `InetAddress.resolver()`
entered a resolver initialization part.
> src/java.base/share/classes/java/net/InetAddress.java line 860:
>
>> 858: return host;
>> 859: }
>> 860: } catch (RuntimeException | UnknownHostException e) {
>
> This may deserve a comment: resolver.lookUpHostName and
> InetAddress.getAllbyName0 delegate to the system-wide resolver, which could
> be custom, and we treat any unexpected RuntimeException thrown by the
> provider at that point as we would treat an UnknownHostException or an
> unmatched host name.
Agree - comment added as part of 648e65b8e6ae9687f7164ea3e81c51ab2b5e2dfe.
> src/java.base/share/classes/java/net/InetAddress.java line 1063:
>
>> 1061: public Stream lookupAddresses(String host,
>> LookupPolicy policy)
>> 1062: throws UnknownHostException {
>> 1063: Objects.requireNonNull(host);
>
> Should we also double check that `policy` is not null? Or maybe assert it?
Yes, we should since custom resolvers might call the built-in one with `null` -
added non-null check in 648e65b .
> src/java.base/share/classes/java/net/InetAddress.java line 1203:
>
>> 1201: + " as hosts file " + hostsFile + " not found
>> ");
>> 1202: }
>> 1203: // Check number of found addresses:
>
> I wonder if the logic here could be simplified by first checking whether only
> IPv4 addresses are requested, and then if only IPv6 addresses are requested.
>
> That is - evacuate the simple cases first and then only deal with the more
> complex case where you have to combine the two lists...
Tried to simplify it in 648e65b8e6ae9687f7164ea3e81c51ab2b5e2dfe
-
PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v4]
> This change implements a new service provider interface for host name and > address resolution, so that java.net.InetAddress API can make use of > resolvers other than the platform's built-in resolver. > > The following API classes are added to `java.net.spi` package to facilitate > this: > - `InetAddressResolverProvider` - abstract class defining a service, and is, > essentially, a factory for `InetAddressResolver` resolvers. > - `InetAddressResolverProvider.Configuration ` - an interface describing the > platform's built-in configuration for resolution operations that could be > used to bootstrap a resolver construction, or to implement partial delegation > of lookup operations. > - `InetAddressResolver` - an interface that defines methods for the > fundamental forward and reverse lookup operations. > - `InetAddressResolver.LookupPolicy` - a class whose instances describe the > characteristics of one forward lookup operation. > > More details in [JEP-418](https://openjdk.java.net/jeps/418). > > Testing: new and existing `tier1:tier3` tests Aleksei Efimov has updated the pull request incrementally with four additional commits since the last revision: - Remove no longer used import from IPSupport - Rename IPSupport.hasAddress and update it to use SocketChannel.open - Address review comments: javadoc + code cleanup - Address resolver bootstraping issue - Changes: - all: https://git.openjdk.java.net/jdk/pull/5822/files - new: https://git.openjdk.java.net/jdk/pull/5822/files/d302a49a..ac0d2184 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5822=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5822=02-03 Stats: 245 lines in 5 files changed: 180 ins; 35 del; 30 mod Patch: https://git.openjdk.java.net/jdk/pull/5822.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5822/head:pull/5822 PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v3]
On Sat, 16 Oct 2021 10:48:32 GMT, Mark Sheppard wrote: > What’s in a name? I find the method names of the InetAddressResolver > interface a bit ambiguous. Typically in this DNS problem space one speaks of > lookup to resolve a hostname to its associated addresses and reverse lookup > to resolve an IP address to a hostname (or names) associated with it. I'm not sure that I see an ambiguity here: Interface has two methods, both are for lookup operations. That is why `lookup` word is used in both names. Then we put a description of a returned result after operation name: `Addresses` and `HostName` are the results. In cases when we want to highlight a parameter type in a method name we can use ‘by’/‘with’ prepositions: for instance, `InetAddress.getByName` `InetAddress.getByAddress` `ScheduledExecutorService.scheduleWithFixedDelay`. We can try and apply this approach to the description of DNS operations above: > lookup to resolve a hostname to its associated addresses operation: `lookup` result: `addresses` -> methodName: `lookupAddresses` > reverse lookup to resolve an IP address to a hostname operation: `lookup` (we've dropped reverse word here) result: `hostname` -> methodName: `lookupHostName` - PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v3]
On Sat, 16 Oct 2021 12:30:44 GMT, Mark Sheppard wrote: > So Suggestion is refector remove Configuration to simplify the interface and > provide the BULITIN_RESOLVER and hostname as parameters to the > InetAddressResolverProvider::get method During work on this JEP we've examined the approach suggested above, and there are few issues with it: - `InetAddressResolverProvider.Configuration` interface API might evolve, e.g. there might be a need to pass additional configuration items. - local hostname is a dynamic information, therefore it cannot be passed as string parameter to `InetAddressResolverProvider::get`. It's been also discussed in the CSR comments [here](https://bugs.openjdk.java.net/browse/JDK-8274558) - PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v3]
> This change implements a new service provider interface for host name and > address resolution, so that java.net.InetAddress API can make use of > resolvers other than the platform's built-in resolver. > > The following API classes are added to `java.net.spi` package to facilitate > this: > - `InetAddressResolverProvider` - abstract class defining a service, and is, > essentially, a factory for `InetAddressResolver` resolvers. > - `InetAddressResolverProvider.Configuration ` - an interface describing the > platform's built-in configuration for resolution operations that could be > used to bootstrap a resolver construction, or to implement partial delegation > of lookup operations. > - `InetAddressResolver` - an interface that defines methods for the > fundamental forward and reverse lookup operations. > - `InetAddressResolver.LookupPolicy` - a class whose instances describe the > characteristics of one forward lookup operation. > > More details in [JEP-418](https://openjdk.java.net/jeps/418). > > Testing: new and existing `tier1:tier3` tests Aleksei Efimov has updated the pull request incrementally with two additional commits since the last revision: - Add @since tags to new API classes - Add checks and test for empty stream resolver results - Changes: - all: https://git.openjdk.java.net/jdk/pull/5822/files - new: https://git.openjdk.java.net/jdk/pull/5822/files/41717fc7..d302a49a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5822=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5822=01-02 Stats: 150 lines in 6 files changed: 146 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/5822.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5822/head:pull/5822 PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v2]
> This change implements a new service provider interface for host name and > address resolution, so that java.net.InetAddress API can make use of > resolvers other than the platform's built-in resolver. > > The following API classes are added to `java.net.spi` package to facilitate > this: > - `InetAddressResolverProvider` - abstract class defining a service, and is, > essentially, a factory for `InetAddressResolver` resolvers. > - `InetAddressResolverProvider.Configuration ` - an interface describing the > platform's built-in configuration for resolution operations that could be > used to bootstrap a resolver construction, or to implement partial delegation > of lookup operations. > - `InetAddressResolver` - an interface that defines methods for the > fundamental forward and reverse lookup operations. > - `InetAddressResolver.LookupPolicy` - a class whose instances describe the > characteristics of one forward lookup operation. > > More details in [JEP-418](https://openjdk.java.net/jeps/418). > > Testing: new and existing `tier1:tier3` tests 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 two additional commits since the last revision: - Merge branch 'master' into JDK-8244202_JEP418_impl - 8244202: Implementation of JEP 418: Internet-Address Resolution SPI - Changes: - all: https://git.openjdk.java.net/jdk/pull/5822/files - new: https://git.openjdk.java.net/jdk/pull/5822/files/77551538..41717fc7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5822=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5822=00-01 Stats: 4216 lines in 179 files changed: 1929 ins; 1744 del; 543 mod Patch: https://git.openjdk.java.net/jdk/pull/5822.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5822/head:pull/5822 PR: https://git.openjdk.java.net/jdk/pull/5822
RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI
This change implements a new service provider interface for host name and address resolution, so that java.net.InetAddress API can make use of resolvers other than the platform's built-in resolver. The following API classes are added to `java.net.spi` package to facilitate this: - `InetAddressResolverProvider` - abstract class defining a service, and is, essentially, a factory for `InetAddressResolver` resolvers. - `InetAddressResolverProvider.Configuration ` - an interface describing the platform's built-in configuration for resolution operations that could be used to bootstrap a resolver construction, or to implement partial delegation of lookup operations. - `InetAddressResolver` - an interface that defines methods for the fundamental forward and reverse lookup operations. - `InetAddressResolver.LookupPolicy` - a class whose instances describe the characteristics of one forward lookup operation. More details in [JEP-418](https://openjdk.java.net/jeps/418). Testing: new and existing `tier1:tier3` tests - Commit messages: - 8244202: Implementation of JEP 418: Internet-Address Resolution SPI Changes: https://git.openjdk.java.net/jdk/pull/5822/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5822=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8244202 Stats: 2779 lines in 50 files changed: 2524 ins; 134 del; 121 mod Patch: https://git.openjdk.java.net/jdk/pull/5822.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5822/head:pull/5822 PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8273402: Use derived NamingExceptions in com.sun.jndi.ldap.Connection#readReply
On Fri, 10 Sep 2021 17:04:58 GMT, Michael Osipov wrote: >> Hi, >> The following fix changes type of exception thrown when an LDAP operation >> fails for reasons like read timeout or connection closure/cancellation: >> instead of throwing a general `NamingException`, the internal LDAP >> connection class will throw a >> [`CommunicationException`](https://github.com/openjdk/jdk/blob/master/src/java.naming/share/classes/javax/naming/CommunicationException.java#L29) >> that better matches the reasons described. >> >> Testing shows no problem with the proposed fix. > > @AlekseiEfimov Thanks for making this happen so fast. Do you see any chance > to have this backported to LTS releases where this is required? I have > reported the issue initially and like to make this available in Tomcat 8.5+. Hi @michael-o, I'm not involved in the backporting process to older releases. You'll need to find the right contact for a particular release you need a backport for, and ask there. - PR: https://git.openjdk.java.net/jdk/pull/5456
Integrated: 8273402: Use derived NamingExceptions in com.sun.jndi.ldap.Connection#readReply
On Thu, 9 Sep 2021 22:02:55 GMT, Aleksei Efimov wrote: > Hi, > The following fix changes type of exception thrown when an LDAP operation > fails for reasons like read timeout or connection closure/cancellation: > instead of throwing a general `NamingException`, the internal LDAP connection > class will throw a > [`CommunicationException`](https://github.com/openjdk/jdk/blob/master/src/java.naming/share/classes/javax/naming/CommunicationException.java#L29) > that better matches the reasons described. > > Testing shows no problem with the proposed fix. This pull request has now been integrated. Changeset: c464f090 Author:Aleksei Efimov URL: https://git.openjdk.java.net/jdk/commit/c464f09056c239f701b400a5c59c54646f840391 Stats: 41 lines in 3 files changed: 13 ins; 3 del; 25 mod 8273402: Use derived NamingExceptions in com.sun.jndi.ldap.Connection#readReply Reviewed-by: dfuchs - PR: https://git.openjdk.java.net/jdk/pull/5456
RFR: 8273402: Use derived NamingExceptions in com.sun.jndi.ldap.Connection#readReply
Hi, The following fix changes type of exception thrown when an LDAP operation fails for reasons like read timeout or connection closure/cancellation: instead of throwing a general `NamingException`, the internal LDAP connection class will throw a [`CommunicationException`](https://github.com/openjdk/jdk/blob/master/src/java.naming/share/classes/javax/naming/CommunicationException.java#L29) that better matches the reasons described. Testing shows no problem with the proposed fix. - Commit messages: - 8273402: Remove redundant catch statement, change comment to match fix - 8273402: Use derived NamingExceptions in com.sun.jndi.ldap.Connection#readReply Changes: https://git.openjdk.java.net/jdk/pull/5456/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5456=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273402 Stats: 41 lines in 3 files changed: 13 ins; 3 del; 25 mod Patch: https://git.openjdk.java.net/jdk/pull/5456.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5456/head:pull/5456 PR: https://git.openjdk.java.net/jdk/pull/5456
Re: RFR: 8273484: Cleanup unnecessary null comparison before instanceof check in java.naming [v2]
On Thu, 9 Sep 2021 13:24:27 GMT, Andrey Turbanov wrote: >> Update code checks both non-null and instance of a class in java.naming >> module classes. >> The checks and explicit casts could also be replaced with pattern matching >> for the instanceof operator. >> For example: >> The following code: >> >> return (obj != null && >> obj instanceof CompoundName && >> impl.equals(((CompoundName)obj).impl)); >> >> >> Can be simplified to: >> >> >> return (obj instanceof CompoundName other) && >> impl.equals(other.impl); >> >> >> See similar cleanup in java.base - >> https://bugs.openjdk.java.net/browse/JDK-8258422 > > Andrey Turbanov has updated the pull request incrementally with one > additional commit since the last revision: > > 8273484: Cleanup unnecessary null comparison before instanceof check in > java.naming The latest version looks good to me. Our CI system is also happy with this patch - no `java.naming` test failures observed. - Marked as reviewed by aefimov (Committer). PR: https://git.openjdk.java.net/jdk/pull/5374
Re: RFR: 8273484: Cleanup unnecessary null comparison before instanceof check in java.naming
On Mon, 6 Sep 2021 08:08:33 GMT, Andrey Turbanov wrote: > Update code checks both non-null and instance of a class in java.naming > module classes. > The checks and explicit casts could also be replaced with pattern matching > for the instanceof operator. > For example: > The following code: > > return (obj != null && > obj instanceof CompoundName && > impl.equals(((CompoundName)obj).impl)); > > > Can be simplified to: > > > return (obj instanceof CompoundName other) && > impl.equals(other.impl); > > > See similar cleanup in java.base - > https://bugs.openjdk.java.net/browse/JDK-8258422 Hi Andrey, The changes look good to me. And it looks like there is one more method to cleanup: [LdapReferralContext.setHopCount](https://github.com/openjdk/jdk/blob/master/src/java.naming/share/classes/com/sun/jndi/ldap/LdapReferralContext.java#L201) - PR: https://git.openjdk.java.net/jdk/pull/5374
Re: RFR: 8273098: Unnecessary Vector usage in java.naming
On Thu, 26 Aug 2021 07:04:45 GMT, Andrey Turbanov wrote: > Usage of thread-safe collection Vector is unnecessary. It's recommended to > use ArrayList/array if a thread-safe implementation is not needed. In > post-BiasedLocking times, this is gets worse, as every access is synchronized. > I checked only places where Vector was used as local variable. Hi Andrey, The changes look good to me. I've run your patch through our CI and all JNDI tests are green. Best, Aleksei - Marked as reviewed by aefimov (Committer). PR: https://git.openjdk.java.net/jdk/pull/5262
Re: RFR: 8269124: Update java.time to use switch expressions (part II) [v5]
On Thu, 24 Jun 2021 12:01:04 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review the second half of my update for the `java.time` >> package to make use of switch expressions? >> >> This PR was split into two parts due to the large number of files affected. >> >> Kind regards, >> >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8269124: Added instanceof pattern variables Marked as reviewed by aefimov (Committer). - PR: https://git.openjdk.java.net/jdk/pull/4552
Re: RFR: 8269124: Update java.time to use switch expressions (part II) [v3]
On Tue, 22 Jun 2021 17:50:05 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review the second half of my update for the `java.time` >> package to make use of switch expressions? >> >> This PR was split into two parts due to the large number of files affected. >> >> Kind regards, >> >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8269124: Added missing return Marked as reviewed by aefimov (Committer). - PR: https://git.openjdk.java.net/jdk/pull/4552
Re: RFR: 8269124: Update java.time to use switch expressions (part II) [v2]
On Tue, 22 Jun 2021 16:07:12 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review the second half of my update for the `java.time` >> package to make use of switch expressions? >> >> This PR was split into two parts due to the large number of files affected. >> >> Kind regards, >> >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8269124: Added missing brace; fixed build issue Thanks for quickly addressing the issues. The latest version looks good to me. - Marked as reviewed by aefimov (Committer). PR: https://git.openjdk.java.net/jdk/pull/4552
Re: RFR: 8269124: Update java.time to use switch expressions (part II)
On Tue, 22 Jun 2021 10:50:17 GMT, Patrick Concannon
wrote:
> Hi,
>
> Could someone please review the second half of my update for the `java.time`
> package to make use of switch expressions?
>
> This PR was split into two parts due to the large number of files affected.
>
> Kind regards,
>
> Patrick
Changes requested by aefimov (Committer).
src/java.base/share/classes/java/time/Instant.java line 852:
> 850: public Instant plus(long amountToAdd, TemporalUnit unit) {
> 851: if (unit instanceof ChronoUnit) {
> 852: switch ((ChronoUnit) unit) {
It looks like we need to put `return` here, otherwise
`UnsupportedTemporalTypeException` will be thrown every time the method is
called:
jshell> Instant.now()
$2 ==> 2021-06-22T15:35:28.771128667Z
jshell> $2.plus(10L, ChronoUnit.SECONDS)
| Exception java.time.temporal.UnsupportedTemporalTypeException: Unsupported
unit: Seconds
|at Instant.plus (Instant.java:862)
|at (#5:1)
src/java.base/share/classes/java/time/ZonedDateTime.java line 1311:
> 1309: }
> 1310: default -> resolveLocal(dateTime.with(field, newValue));
> 1311: };
It looks like that the closing braces `}` is missing here.
-
PR: https://git.openjdk.java.net/jdk/pull/4552
Integrated: 8265309: com/sun/jndi/dns/ConfigTests/Timeout.java fails with "Address already in use" BindException
On Thu, 27 May 2021 15:02:01 GMT, Aleksei Efimov wrote: > Hi, > > `com/sun/jndi/dns/ConfigTests/Timeout.java ` was seen failing intermittently > with "Address already in use" `BindException`. The reason of this failure is > that `disconnect` call in JNDI `DnsClient` fails to rebind the datagram > socket to the original port during `disconnect` call (the failure is in > `DatagramChannel::repairSocket`). > Since Datagram socket is not reused and closed after the failure `finally` > block with the `disconnect` call can be removed. > > The fix was tested with all available JNDI/DNS tests, and no failures related > to the change were observed. This pull request has now been integrated. Changeset: 0c0ff7fb Author:Aleksei Efimov URL: https://git.openjdk.java.net/jdk/commit/0c0ff7fb0c1ff45ebaee863f73902cab1e9de4f3 Stats: 31 lines in 2 files changed: 0 ins; 5 del; 26 mod 8265309: com/sun/jndi/dns/ConfigTests/Timeout.java fails with "Address already in use" BindException Reviewed-by: dfuchs - PR: https://git.openjdk.java.net/jdk/pull/4227
Re: RFR: 8265309: com/sun/jndi/dns/ConfigTests/Timeout.java fails with "Address already in use" BindException [v2]
On Thu, 27 May 2021 15:33:00 GMT, Daniel Fuchs wrote: > LGTM. I really wish git had a better `diff` ! > Can you add `@bug 8265309` to com/sun/jndi/dns/ConfigTests/Timeout.java since > this is a product change? Thanks for the review and the suggestion @dfuch . Bug ID has been added. - PR: https://git.openjdk.java.net/jdk/pull/4227
Re: RFR: 8265309: com/sun/jndi/dns/ConfigTests/Timeout.java fails with "Address already in use" BindException [v2]
> Hi, > > `com/sun/jndi/dns/ConfigTests/Timeout.java ` was seen failing intermittently > with "Address already in use" `BindException`. The reason of this failure is > that `disconnect` call in JNDI `DnsClient` fails to rebind the datagram > socket to the original port during `disconnect` call (the failure is in > `DatagramChannel::repairSocket`). > Since Datagram socket is not reused and closed after the failure `finally` > block with the `disconnect` call can be removed. > > The fix was tested with all available JNDI/DNS tests, and no failures related > to the change were observed. Aleksei Efimov has updated the pull request incrementally with one additional commit since the last revision: Add bug id to Timeout.java jtreg header - Changes: - all: https://git.openjdk.java.net/jdk/pull/4227/files - new: https://git.openjdk.java.net/jdk/pull/4227/files/f3671fc5..82d026db Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4227=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4227=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4227.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4227/head:pull/4227 PR: https://git.openjdk.java.net/jdk/pull/4227
RFR: 8265309: com/sun/jndi/dns/ConfigTests/Timeout.java fails with "Address already in use" BindException
Hi, `com/sun/jndi/dns/ConfigTests/Timeout.java ` was seen failing intermittently with "Address already in use" `BindException`. The reason of this failure is that `disconnect` call in JNDI `DnsClient` fails to rebind the datagram socket to the original port during `disconnect` call (the failure is in `DatagramChannel::repairSocket`). Since Datagram socket is not reused and closed after the failure `finally` block with the `disconnect` call can be removed. The fix was tested with all available JNDI/DNS tests, and no failures related to the change were observed. - Commit messages: - 8265309: com/sun/jndi/dns/ConfigTests/Timeout.java fails with "Address already in use" BindException Changes: https://git.openjdk.java.net/jdk/pull/4227/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4227=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8265309 Stats: 30 lines in 1 file changed: 0 ins; 5 del; 25 mod Patch: https://git.openjdk.java.net/jdk/pull/4227.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4227/head:pull/4227 PR: https://git.openjdk.java.net/jdk/pull/4227
Re: RFR: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI [v3]
On Tue, 13 Apr 2021 10:04:19 GMT, Conor Cleary wrote: >> ### Description >> This fix is part of a previous effort to both cleanup/modernise JNDI code, >> the details of which can be seen in >> [JDK-8048091](https://bugs.openjdk.java.net/browse/JDK-8048091). A number >> JNDI methods under `java.naming` use Anonymous Inner Classes in cases where >> only a single object unique to the requirements of the method is used. The >> issues these occurrences of AICs cause are highlighted below. >> >> - AICs, in the cases concerned with this fix, are used where only one >> operation is required. While AICs can be useful for more complex >> implementations (using interfaces, multiple methods needed, local fields >> etc.), Lambdas are better suited here as they result in a more readable and >> concise solution. >> >> ### Fixes >> - Where applicable, occurrences of AICs were replaced with lambdas to >> address the issues above resulting primarily in more readable/concise code. > > Conor Cleary has updated the pull request incrementally with one additional > commit since the last revision: > > 8048199: Cleaner syntak in getContextClassLoader Marked as reviewed by aefimov (Committer). - PR: https://git.openjdk.java.net/jdk/pull/3416
Re: RFR: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI [v2]
On Mon, 12 Apr 2021 14:09:55 GMT, Conor Cleary wrote:
>> ### Description
>> This fix is part of a previous effort to both cleanup/modernise JNDI code,
>> the details of which can be seen in
>> [JDK-8048091](https://bugs.openjdk.java.net/browse/JDK-8048091). A number
>> JNDI methods under `java.naming` use Anonymous Inner Classes in cases where
>> only a single object unique to the requirements of the method is used. The
>> issues these occurrences of AICs cause are highlighted below.
>>
>> - AICs, in the cases concerned with this fix, are used where only one
>> operation is required. While AICs can be useful for more complex
>> implementations (using interfaces, multiple methods needed, local fields
>> etc.), Lambdas are better suited here as they result in a more readable and
>> concise solution.
>>
>> ### Fixes
>> - Where applicable, occurrences of AICs were replaced with lambdas to
>> address the issues above resulting primarily in more readable/concise code.
>
> Conor Cleary has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Update copyright headers
> - Tidied up lambdas
The change looks good to me with one small suggestion:
src/java.naming/share/classes/javax/naming/ldap/StartTlsRequest.java line 223:
> 221: */
> 222: private final ClassLoader getContextClassLoader() {
> 223: PrivilegedAction pa = () ->
> Thread.currentThread().getContextClassLoader();
We can use here an instance method reference to beautify code a little bit more:
```PrivilegedAction pa =
Thread.currentThread()::getContextClassLoader;```
-
Marked as reviewed by aefimov (Committer).
PR: https://git.openjdk.java.net/jdk/pull/3416
Integrated: 8264048: Fix caching in Jar URL connections when an entry is missing
On Tue, 30 Mar 2021 11:30:48 GMT, Aleksei Efimov wrote: > Current fix tries to tackle an issue with URL connection referencing > non-existing Jar file entries: > If an entry that doesn't exist is specified in an URL connection the > underlying Jar file is still cached even if an exception is thrown after > that. Such behavior prevents the caller, for instance, a `URLClassLoader`, > from closing a Jar file. > > The proposed fix checks if entry exists before caching a Jar file (only for > cases with enabled caching): > - If entry exists - jar file is cached if it wasn't cached before > - If entry doesn't exist and jar file wasn't cached before - jar file is > closed and exception is thrown > - If entry doesn't exist and jar file was cached before - jar file is kept > cached and exception is thrown > > > The following tests have been used to verify the fix: > - New regression tests > - ``:jdk_core:`` tests > - `api/java_util`,`api/java_net` JCK tests This pull request has now been integrated. Changeset: a611c462 Author:Aleksei Efimov URL: https://git.openjdk.java.net/jdk/commit/a611c462 Stats: 372 lines in 6 files changed: 346 ins; 2 del; 24 mod 8264048: Fix caching in Jar URL connections when an entry is missing Co-authored-by: Daniel Fuchs Reviewed-by: bchristi, dfuchs - PR: https://git.openjdk.java.net/jdk/pull/3263
Re: RFR: 8264048: Fix caching in Jar URL connections when an entry is missing [v2]
> Current fix tries to tackle an issue with URL connection referencing > non-existing Jar file entries: > If an entry that doesn't exist is specified in an URL connection the > underlying Jar file is still cached even if an exception is thrown after > that. Such behavior prevents the caller, for instance, a `URLClassLoader`, > from closing a Jar file. > > The proposed fix checks if entry exists before caching a Jar file (only for > cases with enabled caching): > - If entry exists - jar file is cached if it wasn't cached before > - If entry doesn't exist and jar file wasn't cached before - jar file is > closed and exception is thrown > - If entry doesn't exist and jar file was cached before - jar file is kept > cached and exception is thrown > > > The following tests have been used to verify the fix: > - New regression tests > - ``:jdk_core:`` tests > - `api/java_util`,`api/java_net` JCK tests Aleksei Efimov has updated the pull request incrementally with one additional commit since the last revision: JDK-8264048: Remove old version of RemoveJar test - Changes: - all: https://git.openjdk.java.net/jdk/pull/3263/files - new: https://git.openjdk.java.net/jdk/pull/3263/files/6aeb..2f0fa527 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3263=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3263=00-01 Stats: 179 lines in 1 file changed: 0 ins; 179 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3263.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3263/head:pull/3263 PR: https://git.openjdk.java.net/jdk/pull/3263
Re: RFR: 8264048: Fix caching in Jar URL connections when an entry is missing
On Wed, 31 Mar 2021 11:24:02 GMT, Daniel Fuchs wrote: >> Current fix tries to tackle an issue with URL connection referencing >> non-existing Jar file entries: >> If an entry that doesn't exist is specified in an URL connection the >> underlying Jar file is still cached even if an exception is thrown after >> that. Such behavior prevents the caller, for instance, a `URLClassLoader`, >> from closing a Jar file. >> >> The proposed fix checks if entry exists before caching a Jar file (only for >> cases with enabled caching): >> - If entry exists - jar file is cached if it wasn't cached before >> - If entry doesn't exist and jar file wasn't cached before - jar file is >> closed and exception is thrown >> - If entry doesn't exist and jar file was cached before - jar file is kept >> cached and exception is thrown >> >> >> The following tests have been used to verify the fix: >> - New regression tests >> - ``:jdk_core:`` tests >> - `api/java_util`,`api/java_net` JCK tests > > Hi Aleksei, thanks for putting this together. > > `test/jdk/sun/misc/URLClassPath/RemoveJar.java` seems to be an older version > of `test/jdk/java/net/URLClassLoader/RemoveJar.java`. The two tests are > almost identical - so `test/jdk/sun/misc/URLClassPath/RemoveJar.java` can > probably be removed from the PR. > > Otherwise the proposed changes look good to me. Thanks for the review, Daniel. It is correct that `test/jdk/sun/misc/URLClassPath/RemoveJar.java` is an old version. It is removed now. - PR: https://git.openjdk.java.net/jdk/pull/3263
RFR: 8264048: Fix caching in Jar URL connections when an entry is missing
Current fix tries to tackle an issue with URL connection referencing non-existing Jar file entries: If an entry that doesn't exist is specified in an URL connection the underlying Jar file is still cached even if an exception is thrown after that. Such behavior prevents the caller, for instance, a `URLClassLoader`, from closing a Jar file. The proposed fix checks if entry exists before caching a Jar file (only for cases with enabled caching): - If entry exists - jar file is cached if it wasn't cached before - If entry doesn't exist and jar file wasn't cached before - jar file is closed and exception is thrown - If entry doesn't exist and jar file was cached before - jar file is kept cached and exception is thrown The following tests have been used to verify the fix: - New regression tests - ``:jdk_core:`` tests - `api/java_util`,`api/java_net` JCK tests - Commit messages: - Merge remote-tracking branch 'origin' into JDK-8264048 - 8264048: Fix caching in Jar URL connections when an entry is missing Changes: https://git.openjdk.java.net/jdk/pull/3263/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3263=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264048 Stats: 551 lines in 7 files changed: 525 ins; 2 del; 24 mod Patch: https://git.openjdk.java.net/jdk/pull/3263.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3263/head:pull/3263 PR: https://git.openjdk.java.net/jdk/pull/3263
Re: RFR: 8263855: Use the blessed modifier order in java.management/naming [v2]
On Fri, 19 Mar 2021 17:13:58 GMT, Alex Blewitt wrote: >> As with #2993 changing the order of `final static` to `static final` for the >> `java.management`, `java.management.rmi` and `java.naming` modules. > > Alex Blewitt has updated the pull request incrementally with one additional > commit since the last revision: > > Added more replacements of 'final public' with 'public final' `java.naming` module changes look good to me. - Marked as reviewed by aefimov (Committer). PR: https://git.openjdk.java.net/jdk/pull/3078
Re: RFR: 8263509: LdapSchemaParser.readNextTag checks array length incorrectly
On Tue, 16 Mar 2021 08:56:29 GMT, Aleksey Shipilev wrote: >> Seems fine. Lets hope no caller relies on this throwing AIOOBE. >> >> ..Thomas > >> This looks right but I'm surprised it isn't caught by tests (@AlekseiEfimov >> - do you have suggests for tests that would be useful to add here?) > > Can we go without adding tests here? This seems quite trivial. Hi @shipilev, We do not have tests for LDAP schema parser, and creating one wouldn't be trivial: environment with real LDAP server will be needed for, at least, to collect LDAP message dumps with required schema value. Therefore, I think it is reasonable to continue without a test here. -Aleksei - PR: https://git.openjdk.java.net/jdk/pull/2968
Re: RFR: 8263509: LdapSchemaParser.readNextTag checks array length incorrectly
On Fri, 12 Mar 2021 13:25:31 GMT, Aleksey Shipilev wrote:
> SonarCloud rightfully says:
> The length of "values" is always ">=0", so update this test to either "==0"
> or ">0".
>
> // make sure at least one value was returned
> if(values.length < 0) { // <--- here
> throw new InvalidAttributeValueException("no values for " +
> "attribute "" +
> tagName + """);
> }
>
> There is a subsequent access to values[0], which means the failure would
> throw `AIOOB`, not `InvalidAttributeValueException`.
>
> Additional testing:
> - [x] Linux x86_64 fastdebug, `com/sun/jndi`
Marked as reviewed by aefimov (Committer).
-
PR: https://git.openjdk.java.net/jdk/pull/2968
Re: RFR: 8260506: VersionHelper cleanup
On Wed, 27 Jan 2021 16:03:48 GMT, Daniel Fuchs wrote: >> Some small modernizations and cleanups that ultimately help startup of apps >> using JNDI/InitialContext > > Looks fine to me. I have satisfied myself that `c` couldn't be a primitive > type if we reach here. > Please make sure to run tier2 for this one too. The changes looks good to me. - PR: https://git.openjdk.java.net/jdk/pull/2259
Re: RFR: 8260506: VersionHelper cleanup
On Wed, 27 Jan 2021 12:15:43 GMT, Claes Redestad wrote: > Some small modernizations and cleanups that ultimately help startup of apps > using JNDI/InitialContext Marked as reviewed by aefimov (Committer). - PR: https://git.openjdk.java.net/jdk/pull/2259
Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension [v4]
On Fri, 22 Jan 2021 14:42:10 GMT, Alexey Bakhtin wrote: >> Please review a small patch to enable LDAP TLS Channel Binding with StartTLS >> Extension. >> Test from the bug report and jtreg javax/naming tests are passed. > > Alexey Bakhtin has updated the pull request incrementally with one additional > commit since the last revision: > > Update copyright year Marked as reviewed by aefimov (Committer). - PR: https://git.openjdk.java.net/jdk/pull/2085
Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension [v3]
On Thu, 21 Jan 2021 19:57:04 GMT, Alexey Bakhtin wrote: >> Please review a small patch to enable LDAP TLS Channel Binding with StartTLS >> Extension. >> Test from the bug report and jtreg javax/naming tests are passed. > > Alexey Bakhtin has updated the pull request incrementally with one additional > commit since the last revision: > > Add comments and volatile modifier for tlsHandshakeListener Hi Alexey, The latest changes look good to me. Thanks for handling a case of sequential StartTLS requests on one LDAP context and running the modified test. I've also checked that existing LDAP tests shows no failures with the proposed changed. You might also want to update last modification years to `2021` in both files. - Marked as reviewed by aefimov (Committer). PR: https://git.openjdk.java.net/jdk/pull/2085
Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension [v2]
On Thu, 21 Jan 2021 13:13:38 GMT, Alexey Bakhtin wrote:
>> Please review a small patch to enable LDAP TLS Channel Binding with StartTLS
>> Extension.
>> Test from the bug report and jtreg javax/naming tests are passed.
>
> Alexey Bakhtin has updated the pull request incrementally with one additional
> commit since the last revision:
>
> separate tlsHandshakeCompleted for every StartTLS connection
src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 1034:
> 1032: }
> 1033:
> 1034: private HandshakeListener tlsHandshakeListener;
I believe that `volatile` modifier should be added here. And it could be
beneficial for future maintainers to have here a comment block with a brief
description of when `tlsHandshakeListener` is used.
src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 1059:
> 1057: private class HandshakeListener implements
> HandshakeCompletedListener {
> 1058:
> 1059: private CompletableFuture
> tlsHandshakeCompleted =
`tlsHandshakeCompleted` could be made `final`
-
PR: https://git.openjdk.java.net/jdk/pull/2085
Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension
On Wed, 20 Jan 2021 14:41:26 GMT, Daniel Fuchs wrote:
>> Please review a small patch to enable LDAP TLS Channel Binding with StartTLS
>> Extension.
>> Test from the bug report and jtreg javax/naming tests are passed.
>
> That look reasonable to me. But what would happen if at some point after
> performing some LDAP operations, you called StartTLSResponse::close and then
> after some more time you tried to again create a StartTLSRequest on the same
> context? Would that work - or would you be using a possibly obsolete channel
> binding obtained from the first upgrade?
The change looks valid to me too.
I believe Daniel question could be illustrated with the following change to
`CBwithTLS` reproducer attached to the bug report:
--- CBwithTLS_Original.java 2021-01-20 14:56:09.620844903 +
+++ CBwithTLS.java 2021-01-20 15:01:47.253982227 +
@@ -45,7 +45,7 @@
System.out.println(ctxt.getAttributes("", new
String[]{"defaultNamingContext"}).get("defaultNamingContext").get());
// Switch to TLS
-
+for (int i=0; i<2; i++) {
StartTlsResponse tls = (StartTlsResponse) ctxt.extendedOperation(new
StartTlsRequest());
tls.negotiate();
@@ -64,6 +64,9 @@
lc.login();
Subject.doAs(lc.getSubject(), (PrivilegedAction) CBwithTLS::run);
+lc.logout();
+tls.close();
+}
}
private static Void run() {
-
PR: https://git.openjdk.java.net/jdk/pull/2085
Re: RFR: 8258422: Cleanup unnecessary null comparison before instanceof check in java.base
On Mon, 11 Jan 2021 17:12:24 GMT, Andrey Turbanov wrote: >> Hi @turbanoff, >> This PR is ready for integration - `JDK-8258657` has been resolved. >> You can proceed with issuing `integrate` bot command. Then I will `sponsor` >> it. >> But before that, I would like to ask if you would like to take on board the >> changes to `HttpClientImpl`? Alternatively, I can follow it up separately >> and reapply the backed-out change under different bugID. > > @AlekseiEfimov `HttpClientImpl` is not in `java.base` module. So I think it's > better to not touch it under this PR. To double check that docs build will be stable after integration the following actions have been performed: - The pull request branch was locally synced-up with the latest changes from `master` (`JDK-8258657` is part of them) - Verified that local `docs-reference-api-javadoc` build completes successfully - PR: https://git.openjdk.java.net/jdk/pull/20
Re: RFR: 8258422: Cleanup unnecessary null comparison before instanceof check in java.base
On Mon, 11 Jan 2021 23:29:53 GMT, Aleksei Efimov wrote: >> @AlekseiEfimov `HttpClientImpl` is not in `java.base` module. So I think >> it's better to not touch it under this PR. > > To double check that docs build will be stable after integration the > following actions have been performed: > - The pull request branch was locally synced-up with the latest changes from > `master` (`JDK-8258657` is part of them) > - Verified that local `docs-reference-api-javadoc` build completes > successfully sponsor - PR: https://git.openjdk.java.net/jdk/pull/20
Re: RFR: 8258422: Cleanup unnecessary null comparison before instanceof check in java.base
On Sat, 5 Sep 2020 18:55:53 GMT, Andrey Turbanov wrote: > 8258422: Cleanup unnecessary null comparison before instanceof check in > java.base Hi @turbanoff, This PR is ready for integration - `JDK-8258657` has been resolved. You can proceed with issuing `integrate` bot command. Then I will `sponsor` it. But before that, I would like to ask if you would like to take on board the changes to `HttpClientImpl`? Alternatively, I can follow it up separately and reapply the backed-out change under different bugID. - PR: https://git.openjdk.java.net/jdk/pull/20
Re: RFR: 8258422: Cleanup unnecessary null comparison before instanceof check in java.base [v4]
On Sat, 19 Dec 2020 10:29:23 GMT, Chris Hegarty wrote: >> Thank you for checking `HttpURLConnection` and `Socket`. >> The latest version looks good to me. > > This issue is blocked by [8258657][1]. It should not be integrated until > after 8258657 has been resolved. The JIRA issues have been linked up to > reflect this. > > [1]: https://bugs.openjdk.java.net/browse/JDK-8258657 [JDK-8258657](https://bugs.openjdk.java.net/browse/JDK-8258657) has been resolved. The changes reverted by [JDK-8258696](https://bugs.openjdk.java.net/browse/JDK-8258696) could also be re-applied to `HttpClientImpl` class. - PR: https://git.openjdk.java.net/jdk/pull/20
Re: RFR: 8258422: Cleanup unnecessary null comparison before instanceof check in java.base [v4]
On Wed, 16 Dec 2020 10:32:12 GMT, Andrey Turbanov wrote: >> 8258422: Cleanup unnecessary null comparison before instanceof check in >> java.base > > Andrey Turbanov has updated the pull request incrementally with one > additional commit since the last revision: > > 8258422: Cleanup unnecessary null comparison before instanceof check in > java.base > take advantage of "flow scoping" to eliminate casts Thank you for checking `HttpURLConnection` and `Socket`. The latest version looks good to me. - Marked as reviewed by aefimov (Committer). PR: https://git.openjdk.java.net/jdk/pull/20
Re: RFR: 8258422: Cleanup unnecessary null comparison before instanceof check in java.base [v3]
On Wed, 16 Dec 2020 09:44:37 GMT, Chris Hegarty wrote:
>> Andrey Turbanov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8258422: Cleanup unnecessary null comparison before instanceof check in
>> java.base
>> use instanceof pattern matching in UnixPath too
>
> Let's take advantage of "flow scoping" to eliminate some of these casts. A
> few examples follow.
Hi Andrey,
Could you, please, also take a look at `java.net.Socket`:
java/net/Socket.java:if (bindpoint != null && (!(bindpoint instanceof
InetSocketAddress)))
java/net/Socket.java-throw new
IllegalArgumentException("Unsupported address type");
And `HttpURLConnection`:
sun/net/www/protocol/http/HttpURLConnection.java:if (a
!= null && c instanceof HttpURLConnection) {
sun/net/www/protocol/http/HttpURLConnection.java-
((HttpURLConnection)c).setAuthenticator(a);
The following cmd was used to find them: `rgrep -A 1 "= null .* instanceof "`
-
PR: https://git.openjdk.java.net/jdk/pull/20
Re: RFR: 8258422: Cleanup unnecessary null comparison before instanceof check in java.base
On Wed, 2 Dec 2020 20:15:02 GMT, Andrey Turbanov
wrote:
>> This seems like a reasonable change, which improves readability.
>>
>> My preference is to wait a little longer (hopefully no more than a couple of
>> weeks), until [JEP 394](https://openjdk.java.net/jeps/394) - "Pattern
>> Matching for instanceof" is finalised, then we can remove the explicit casts
>> in many of these cases. For example:
>>
>> --- a/src/java.base/share/classes/java/io/File.java
>> +++ b/src/java.base/share/classes/java/io/File.java
>> @@ -2191,8 +2191,8 @@ public class File
>> * {@code false} otherwise
>> */
>> public boolean equals(Object obj) {
>> -if ((obj != null) && (obj instanceof File)) {
>> -return compareTo((File)obj) == 0;
>> +if (obj instanceof File file) {
>> +return compareTo(file) == 0;
>> }
>> return false;
>> }
>
> Related issue - https://bugs.openjdk.java.net/browse/JDK-8257448
Hi @turbanoff,
I've logged a JBS issue for tracking this change:
https://bugs.openjdk.java.net/browse/JDK-8258422
JEP 394 is finalized now, so I guess you could follow-up Chris suggestion to
remove the explicit casts.
After the fix is properly reviewed and marked as ready for the integration
(you'll need to issue `/integrate` command), once it is done I would happily
`/sponsor` the change.
With Best Regards,
Aleksei
-
PR: https://git.openjdk.java.net/jdk/pull/20
Re: RFR: 8256037: [TESTBUG] com/sun/jndi/dns/ConfigTests/PortUnreachable.java fails due to the hard coded threshold is small
On Mon, 9 Nov 2020 09:19:44 GMT, Jie Fu wrote: > Hi all, > > May I get reviews for this change? > > com/sun/jndi/dns/ConfigTests/PortUnreachable.java fails occasionally on some > of our testing platforms. > The reason is that the hard coded Threshold (1000 ms) [1] is a little smaller > than that (e.g., 1026 ms) on our platforms. > > The fix just increases the threshold from 1000 ms to 3000 ms. > Any comments? > > Thanks. > Best regards, > Jie > > [1] > https://github.com/openjdk/jdk/blob/master/test/jdk/com/sun/jndi/dns/ConfigTests/PortUnreachable.java#L49 Hi @DamonFool, Change looks good to me since 3 seconds is less than 15 seconds of the full timeout. The full timeout interval which is mentioned in the test summary is 15 s: 4 retries with initial timeout set to 1s: 1 + 2 + 4 + 8 = 15 s - Marked as reviewed by aefimov (Committer). PR: https://git.openjdk.java.net/jdk/pull/1116
Integrated: 8251188: Update LDAP tests not to use wildcard addresses
On Fri, 18 Sep 2020 12:59:07 GMT, Aleksei Efimov wrote: > Hi, > > Please help to review > [JDK-8251188](https://bugs.openjdk.java.net/browse/JDK-8251188) fix which > helps to improve LDAP > tests stability. The list of changes: 1. Usages of wildcard address have been > replaced with loopback address. This > change includes addition of `LDAPTestUtils.initEnv` method that takes LDAP > provider URL as a parameter. 2. > `DeadServerTimeoutSSLTest.java` was also updated to fix the intermittent > failures reported by [JDK-8152654 > ](https://bugs.openjdk.java.net/browse/JDK-8152654) and > [JDK-8169942](https://bugs.openjdk.java.net/browse/JDK-8169942). Before the > fix the failure rate was 1 out of 4 runs. > After the fix it was executed 400+ times alongside to other LDAP tests, and > showed no failures, and therefore removed > from the problem list. Thank you, Aleksei This pull request has now been integrated. Changeset: a75edc29 Author:Aleksei Efimov URL: https://git.openjdk.java.net/jdk/commit/a75edc29 Stats: 287 lines in 6 files changed: 216 ins; 29 del; 42 mod 8251188: Update LDAP tests not to use wildcard addresses Reviewed-by: dfuchs - PR: https://git.openjdk.java.net/jdk/pull/252
Re: RFR: 8251188: Update LDAP tests not to use wildcard addresses [v2]
On Tue, 22 Sep 2020 13:39:38 GMT, Aleksei Efimov wrote:
>> test/jdk/com/sun/jndi/ldap/DeadSSLLdapTimeoutTest.java line 171:
>>
>>> 169: System.err.println("Server socket. Failure to accept
>>> connection:");
>>> 170: e.printStackTrace();
>>> 171: }
>>
>> I wonder if removing the while (true) loop will make the test more
>> susceptible of failing in timeout if the server ever
>> receives a connection request from an unexpected client (we've seen that
>> happening in the past with networking tests).
>> Is there anyway the server could attempt to verify that the accepted socket
>> is from the expected client, and close it
>> and go back to accepting if it's not? Maybe by looking at the accepted
>> socket remote address & port?
>
> Thanks for the good suggestion Daniel. I will modify it to look at the remote
> socket's address.
Hi Daniel,
Could you please take a look at the new version of DeadSSLLdapTimeoutTest.java
with the following modifications:
**DeadServerTimeoutSSLTest** test was modified to use custom **SocketFactory**
(**DeadSSLSocketFactory**) which tracks
the first opened socket on LDAP client side. This socket is later used on
server side to check if the connection
initiated by test's LDAP client. If the connection was not established by
LDAP client then new accept attempt is
performed.
The test was never seen to fail during 200+ concurrent LDAP tests runs.
-
PR: https://git.openjdk.java.net/jdk/pull/252
Re: RFR: 8251188: Update LDAP tests not to use wildcard addresses [v2]
> Hi, > > Please help to review > [JDK-8251188](https://bugs.openjdk.java.net/browse/JDK-8251188) fix which > helps to improve LDAP > tests stability. The list of changes: 1. Usages of wildcard address have been > replaced with loopback address. This > change includes addition of `LDAPTestUtils.initEnv` method that takes LDAP > provider URL as a parameter. 2. > `DeadServerTimeoutSSLTest.java` was also updated to fix the intermittent > failures reported by [JDK-8152654 > ](https://bugs.openjdk.java.net/browse/JDK-8152654) and > [JDK-8169942](https://bugs.openjdk.java.net/browse/JDK-8169942). Before the > fix the failure rate was 1 out of 4 runs. > After the fix it was executed 400+ times alongside to other LDAP tests, and > showed no failures, and therefore removed > from the problem list. Thank you, Aleksei Aleksei Efimov has updated the pull request incrementally with one additional commit since the last revision: Update DeadServerTimeoutSSLTest to track and analyze client connections - Changes: - all: https://git.openjdk.java.net/jdk/pull/252/files - new: https://git.openjdk.java.net/jdk/pull/252/files/606c24a8..81fe70b6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=252=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=252=00-01 Stats: 152 lines in 2 files changed: 140 ins; 1 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/252.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/252/head:pull/252 PR: https://git.openjdk.java.net/jdk/pull/252
Re: RFR: 8245527: LDAP Channel Binding support for Java GSS/Kerberos [v2]
On Tue, 22 Sep 2020 20:19:21 GMT, Alexey Bakhtin wrote: >> Hi, >> >> Plaese review JDK-8245527 fix which implements LDAP Channel Binding support >> for Java GSS/Kerberos. >> Initial review is available at core-devs: >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-August/068197.html >> This version removes "tls-unique" CB type from the list of possible channel >> binding types. The only supported type is >> "tls-server-end-point" >> CSR is also updated : https://bugs.openjdk.java.net/browse/JDK-8247311 >> >> Thank you >> Alexey > > Alexey Bakhtin has updated the pull request incrementally with one additional > commit since the last revision: > > 8245527: version.01 Hi Alexey, The latest changes looks good to me. - Marked as reviewed by aefimov (Committer). PR: https://git.openjdk.java.net/jdk/pull/278
Re: RFR: 8251188: Update LDAP tests not to use wildcard addresses
On Tue, 22 Sep 2020 12:30:47 GMT, Daniel Fuchs wrote:
>> Hi,
>>
>> Please help to review
>> [JDK-8251188](https://bugs.openjdk.java.net/browse/JDK-8251188) fix which
>> helps to improve LDAP
>> tests stability. The list of changes: 1. Usages of wildcard address have
>> been replaced with loopback address. This
>> change includes addition of `LDAPTestUtils.initEnv` method that takes LDAP
>> provider URL as a parameter. 2.
>> `DeadServerTimeoutSSLTest.java` was also updated to fix the intermittent
>> failures reported by [JDK-8152654
>> ](https://bugs.openjdk.java.net/browse/JDK-8152654) and
>> [JDK-8169942](https://bugs.openjdk.java.net/browse/JDK-8169942). Before the
>> fix the failure rate was 1 out of 4 runs.
>> After the fix it was executed 400+ times alongside to other LDAP tests, and
>> showed no failures, and therefore removed
>> from the problem list. Thank you, Aleksei
>
> test/jdk/com/sun/jndi/ldap/DeadSSLLdapTimeoutTest.java line 171:
>
>> 169: System.err.println("Server socket. Failure to accept
>> connection:");
>> 170: e.printStackTrace();
>> 171: }
>
> I wonder if removing the while (true) loop will make the test more
> susceptible of failing in timeout if the server ever
> receives a connection request from an unexpected client (we've seen that
> happening in the past with networking tests).
> Is there anyway the server could attempt to verify that the accepted socket
> is from the expected client, and close it
> and go back to accepting if it's not? Maybe by looking at the accepted socket
> remote address & port?
Thanks for the good suggestion Daniel. I will modify it to look at the remote
socket's address and port.
-
PR: https://git.openjdk.java.net/jdk/pull/252
RFR: 8251188: Update LDAP tests not to use wildcard addresses
Hi, Please help to review [JDK-8251188](https://bugs.openjdk.java.net/browse/JDK-8251188) fix which helps to improve LDAP tests stability. The list of changes: 1. Usages of wildcard address have been replaced with loopback address. This change includes addition of `LDAPTestUtils.initEnv` method that takes LDAP provider URL as a parameter. 2. `DeadServerTimeoutSSLTest.java` was also updated to fix the intermittent failures reported by [JDK-8152654 ](https://bugs.openjdk.java.net/browse/JDK-8152654) and [JDK-8169942](https://bugs.openjdk.java.net/browse/JDK-8169942). Before the fix the failure rate was 1 out of 4 runs. After the fix it was executed 400+ times alongside to other LDAP tests, and showed no failures, and therefore removed from the problem list. Thank you, Aleksei - Commit messages: - 8251188: Update LDAP tests not to use wildcard addresses Changes: https://git.openjdk.java.net/jdk/pull/252/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=252=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8251188 Stats: 149 lines in 5 files changed: 76 ins; 28 del; 45 mod Patch: https://git.openjdk.java.net/jdk/pull/252.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/252/head:pull/252 PR: https://git.openjdk.java.net/jdk/pull/252
Re: RFR: 8253155: Minor cleanups and Javadoc fixes for LdapDnsProvider of java.naming
On Tue, 15 Sep 2020 09:39:29 GMT, Daniel Fuchs wrote: >> There are some little flaws in LdapDNSProvider and auxilliary classes, >> mostly in Javadoc. >> >> In detail: >> src/java.naming/share/classes/com/sun/jndi/ldap/DefaultLdapDnsProvider.java: >> Unnecessary import >> src/java.naming/share/classes/com/sun/jndi/ldap/LdapDnsProviderService.java: >> typo >> src/java.naming/share/classes/javax/naming/ldap/spi/LdapDnsProvider.java: >> Whitespace >> src/java.naming/share/classes/javax/naming/ldap/spi/LdapDnsProviderResult.java: >> Spelling of "ldap" -> should be >> capitalized > > Hi Christoph, > > The changes look good to me. > > best regards, > > -- daniel Hi Christoph, Looks good to me. Kind Regards, Aleksei - PR: https://git.openjdk.java.net/jdk/pull/168
Re: RFR: 8253155: Minor cleanups and Javadoc fixes for LdapDnsProvider of java.naming
On Tue, 15 Sep 2020 07:47:54 GMT, Christoph Langer wrote: > There are some little flaws in LdapDNSProvider and auxilliary classes, mostly > in Javadoc. > > In detail: > src/java.naming/share/classes/com/sun/jndi/ldap/DefaultLdapDnsProvider.java: > Unnecessary import > src/java.naming/share/classes/com/sun/jndi/ldap/LdapDnsProviderService.java: > typo > src/java.naming/share/classes/javax/naming/ldap/spi/LdapDnsProvider.java: > Whitespace > src/java.naming/share/classes/javax/naming/ldap/spi/LdapDnsProviderResult.java: > Spelling of "ldap" -> should be > capitalized Marked as reviewed by aefimov (Committer). - PR: https://git.openjdk.java.net/jdk/pull/168
