Feature-Request: HttpResponse.BodyHandlers.ofReader or alternatively convenience function
Hi, while the new HTTP client provides means to read text files as a single string or as a stream of lines, it actually does not provide any way to read the text via the Reader class. For many text types, the character encoding is not fixed by the format but may vary. Thus one cannot easily wrap the input stream of HttpResponse.BodyHandlers.ofInputStream into a Reader. Usually, the character encoding is contained in the content-type header field, but it is not easily extractable if done correctly with whitespace and quote handling. I have seen in the source code, that the implementation does have a method to retrieve the encoding, but this is unfortunately deeply buried and not exposed in the user API. So I would like to suggest to add a HttpResponse.BodyHandler.ofReader method to the API, which takes any charset parameter in the content-type header into account, like the ofString and ofLines methods. Alternatively, convenience methods in HttpHeaders to read the character set (and perhaps the MIME type as well) or a helper class in java.net could be provided. I know that there are quite a few third party implementations, even in the now detached J2EE framework (activation and mail), but I am regarding this as essential enough to be included in the base Java API or at least in the java.net.http module. Thanks for your attention, regards, Klaus
Integrated: 8255584: `HttpPrincipal::getName` returns incorrect name
On Fri, 30 Oct 2020 14:41:18 GMT, Patrick Concannon wrote: > Hi, > > Could someone please review my fix for JDK-8255584: '`HttpPrincipal::getName` > returns incorrect name' ? > The specification for `HttpPrincipal::getName` reports that it should return > the name of the HttpPrincipal in the format "realm:username". However, it > currently returns the username only. > > This fix updates the method to return the name in the correct format as > specified by the javadoc. > > Kind regards, > Patrick This pull request has now been integrated. Changeset: 2b78a43f Author:Patrick Concannon URL: https://git.openjdk.java.net/jdk/commit/2b78a43f Stats: 6 lines in 2 files changed: 4 ins; 0 del; 2 mod 8255584: `HttpPrincipal::getName` returns incorrect name Reviewed-by: dfuchs - PR: https://git.openjdk.java.net/jdk/pull/958
Re: RFR: 8252304: Seed an HttpRequest.Builder from an existing HttpRequest [v2]
> Hi, > > Could someone please review our code for JDK-8252304: 'Seed an > HttpRequest.Builder from an existing HttpRequest'? > > This RFR proposes a new factory method for creating a new `HttpRequest` > builder from an existing `HttpRequest`. > This method can be used to build a new request equivalent to the given > request, but with different attributes. For instance, it will allow the user > to take an existing request and add or change a particular header, provide a > new `URI`, etc. > > > Kind regards, > Patrick & Chris Patrick Concannon has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Merge remote-tracking branch 'origin/master' into JDK-8252304 - Merge remote-tracking branch 'origin/master' into JDK-8252304 - 8252304: Seed an HttpRequest.Builder from an existing HttpRequest - Changes: - all: https://git.openjdk.java.net/jdk/pull/1059/files - new: https://git.openjdk.java.net/jdk/pull/1059/files/e6019f3d..060aafae Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1059&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1059&range=00-01 Stats: 3201 lines in 412 files changed: 1200 ins; 1442 del; 559 mod Patch: https://git.openjdk.java.net/jdk/pull/1059.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1059/head:pull/1059 PR: https://git.openjdk.java.net/jdk/pull/1059
Re: RFR: 8255758: JEP 380 spec clarifications
On Tue, 3 Nov 2020 09:42:05 GMT, Daniel Fuchs wrote: >> I'm not sure, as the system properties don't affect implicit binding, but I >> would have liked to have a place to "explain" all aspects of binding of Unix >> domain sockets/server-sockets. Maybe, I could add a paragraph at the end, >> just as an aside or note. > > Oh - I was assuming that implicit binding & automatic binding were identical > - just had a different trigger. Maybe it would be worth spelling out that > implicit binding is **not** affected by the property then? For my own > curiosity - is it because automatic binding is performed by our java > implementation while implicit binding is performed by the system? These concepts aren't new by the way. "Implicit" binding is when you create a client socket and connect it without calling bind first. This is not possible with server sockets. "Automatic" binding is when you bind a server socket with a null address. All the above is the same for tcp and unix domain. tcp has more options relating to the interface and specifying a port = 0. These cases don't exist for unix domain. Just to add, one other point. It is also possible to automatically bind a client socket (either tcp or unix) (ie bind(null)) There is a small conceptual difference here between tcp and unix. With tcp you get a random port number, but with unix domain you get an unnamed socket address. - PR: https://git.openjdk.java.net/jdk/pull/1021
Re: RFR: 8255758: JEP 380 spec clarifications
On Tue, 3 Nov 2020 09:09:18 GMT, Daniel Fuchs wrote:
>> Minor spec changes from spec approved in initial CSR
>
> src/java.base/share/classes/java/net/doc-files/net-properties.html line 246:
>
>> 244: Unix domain sockets
>> 245: There are a number of system (and networking) properties that affect
>> the behavior of
>> 246: channels to Unix domain server sockets when binding
>> automatically.
>
> Should this also mention implicit binding?
I'm not sure, as the system properties don't affect implicit binding, but I
would have liked to have a place to "explain" all aspects of binding of Unix
domain sockets/server-sockets. Maybe, I could add a paragraph at the end, just
as an aside or note.
> src/java.base/share/classes/java/net/doc-files/net-properties.html line 250:
>
>> 248: Automatic binding of a server socket occurs when {@link
>> 249: java.nio.channels.ServerSocketChannel#bind(SocketAddress,int)
>> ServerSocketChannel.bind}
>> 250: is called with a {@code null} address parameter. In this case, the
>> system
>
> doesn't it also happen when it is called with a UnixDomainSocketAddress that
> has an empty path?
No, an empty path signifies an unnamed address and only client sockets
(SocketChannels) are allowed to be unnamed.
-
PR: https://git.openjdk.java.net/jdk/pull/1021
Re: RFR: 8255758: JEP 380 spec clarifications
On Tue, 3 Nov 2020 09:21:05 GMT, Michael McMahon wrote: >> src/java.base/share/classes/java/net/doc-files/net-properties.html line 246: >> >>> 244: Unix domain sockets >>> 245: There are a number of system (and networking) properties that >>> affect the behavior of >>> 246: channels to Unix domain server sockets when binding >>> automatically. >> >> Should this also mention implicit binding? > > I'm not sure, as the system properties don't affect implicit binding, but I > would have liked to have a place to "explain" all aspects of binding of Unix > domain sockets/server-sockets. Maybe, I could add a paragraph at the end, > just as an aside or note. Oh - I was assuming that implicit binding & automatic binding were identical - just had a different trigger. Maybe it would be worth spelling out that implicit binding is **not** affected by the property then? For my own curiosity - is it because automatic binding is performed by our java implementation while implicit binding is performed by the system? - PR: https://git.openjdk.java.net/jdk/pull/1021
Re: RFR: 8255758: JEP 380 spec clarifications
On Tue, 3 Nov 2020 09:25:52 GMT, Michael McMahon wrote:
>> src/java.base/share/classes/java/net/doc-files/net-properties.html line 250:
>>
>>> 248: Automatic binding of a server socket occurs when {@link
>>> 249: java.nio.channels.ServerSocketChannel#bind(SocketAddress,int)
>>> ServerSocketChannel.bind}
>>> 250: is called with a {@code null} address parameter. In this case, the
>>> system
>>
>> doesn't it also happen when it is called with a UnixDomainSocketAddress that
>> has an empty path?
>
> No, an empty path signifies an unnamed address and only client sockets
> (SocketChannels) are allowed to be unnamed.
You get a BindException if you try to bind a ServerSocketChannel to the unnamed
address. But, the exception message is "unhelpful". I would like to fix that.
-
PR: https://git.openjdk.java.net/jdk/pull/1021
RFR: 8255758: JEP 380 spec clarifications
Minor spec changes from spec approved in initial CSR - Commit messages: - update to net-properties.html - 8255758: JEP 380 spec clarifications Changes: https://git.openjdk.java.net/jdk/pull/1021/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1021&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255758 Stats: 70 lines in 3 files changed: 66 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/1021.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1021/head:pull/1021 PR: https://git.openjdk.java.net/jdk/pull/1021
Re: RFR: 8255758: JEP 380 spec clarifications
On Tue, 3 Nov 2020 09:29:54 GMT, Michael McMahon wrote: >> No, an empty path signifies an unnamed address and only client sockets >> (SocketChannels) are allowed to be unnamed. > > You get a BindException if you try to bind a ServerSocketChannel to the > unnamed address. But, the exception message is "unhelpful". I would like to > fix that. Ah good. I missed that paragraph was only talking about the _server_ sockets. - PR: https://git.openjdk.java.net/jdk/pull/1021
Re: RFR: 8255758: JEP 380 spec clarifications
On Mon, 2 Nov 2020 22:04:56 GMT, Michael McMahon wrote:
> Minor spec changes from spec approved in initial CSR
Marked as reviewed by dfuchs (Reviewer).
src/java.base/share/classes/java/net/doc-files/net-properties.html line 250:
> 248: Automatic binding of a server socket occurs when {@link
> 249: java.nio.channels.ServerSocketChannel#bind(SocketAddress,int)
> ServerSocketChannel.bind}
> 250: is called with a {@code null} address parameter. In this case, the system
doesn't it also happen when it is called with a UnixDomainSocketAddress that
has an empty path?
src/java.base/share/classes/java/net/doc-files/net-properties.html line 246:
> 244: Unix domain sockets
> 245: There are a number of system (and networking) properties that affect
> the behavior of
> 246: channels to Unix domain server sockets when binding
> automatically.
Should this also mention implicit binding?
src/java.base/share/classes/java/net/doc-files/net-properties.html line 252:
> 250: is requested to choose a unique path for the socket in some predefined
> system temporary
> 251: directory.
> 252: There are a number of system (and networking) properties that affect
> the behavior of
nit: `` (lowercase and on a single line to match the style elsewhere)
src/java.base/share/classes/java/net/doc-files/net-properties.html line 271:
> 269: are set, then some systems (eg Windows) may check a commonly used
> environment
> 270: variable as temporary directory.
> 271: {@systemProperty java.io.tmpdir} If the previous step
> fails to locate
nit again (two places): `` should probably be `` especially
since the `` tag is closed with ``
-
PR: https://git.openjdk.java.net/jdk/pull/1021
Re: RFR: 8253005: Add `@throws IOException` in javadoc for `HttpEchange.sendResponseHeaders` [v5]
On Wed, 4 Nov 2020 20:44:10 GMT, Patrick Concannon
wrote:
>> Hi,
>>
>> Could someone please review my fix for JDK-8253005: 'Add `@throws
>> IOException` in javadoc for `HttpEchange.sendResponseHeaders`' ?
>>
>> The method `HttpEchange.sendResponseHeaders` throws an `IOException` but is
>> unspecified in its javadoc. This fix adds an `@throws IOException` to its
>> specification and a description of the conditions under which the exception
>> is thrown.
>>
>> Kind regards,
>> Patrick
>
> Patrick Concannon 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 eight additional
> commits since the last revision:
>
> - 8253005: Added test to verify `IOException` thrown when headers already
> sent
> - Merge remote-tracking branch 'origin/master' into JDK-8253005
> - Merge remote-tracking branch 'origin/master' into JDK-8253005
> - Merge remote-tracking branch 'origin/master' into JDK-8253005
> - Merge remote-tracking branch 'origin/master' into JDK-8253005
> - Merge remote-tracking branch 'origin/master' into JDK-8253005
> - Merge remote-tracking branch 'origin/master' into JDK-8253005
> - 8253005: Add `@throws IOException` in javadoc for
> `HttpEchange.sendResponseHeaders`
Changes requested by dfuchs (Reviewer).
test/jdk/java/net/httpclient/SendResponseHeadersTest.java line 86:
> 84: static class TestHandler implements HttpHandler {
> 85: public void handle(HttpExchange exchange) throws IOException {
> 86: exchange.sendResponseHeaders(200, 0);
It's always preferable to read the request body fully before calling
`sendResponseHeaders`
-
PR: https://git.openjdk.java.net/jdk/pull/1014
RFR: 8246741: NetworkInterface/UniqueMacAddressesTest: mac address uniqueness test failed
The primary goal of this fix was to refactor NetworkInterface/UniqueMacAddressesTest.java to make use of the jdk.test.lib.NetworkConfiguration class instead of java.net.NetworkInterface. This was due to the original failure being related to the use of the 'awdl' interface on macOS, which should be skipped for functional tests. The NetworkConfiguration class accounts for this when returning the list of present interfaces (originally retrieved using NetworkInterface.getNetworkInterfaces()) as well as for other OS specific properties. The effects of these changes can be seen on L59 and L101-106. In addition to these changes, some modifications were made to the test in the interest of keeping it concise and readable but were made bearing in mind the original logging of the test was already up to a good standard (and I sought to keep it that way). To summarize the other changes: - When using NetworkConfiguration::interfaces to read in a stream of NetworkInterface objects, the objects are collected into a list of records (see [JEP 395](https://openjdk.java.net/jeps/395)) to avoid multiple calls to NetworkInterface::getHardwareAddress (which can throw a SocketException) throughout the test. - When checking if the MAC addresses are unique, the testMacAddressesEqual() method is not called unless the interface names are different. If testMacAddressesEqual() returns true, the calling method will simply return instead of updating the value of a boolean variable. - Majority of logging was moved to the testMacAddressesEqualMethod() so that only logs for comparisons are carried out. There is scope for additional logging of, for example, the list of present interfaces returned by createNetworkInterfaceList() if deemed necessary by review. - Commit messages: - 8246741: NetworkInterface/UniqueMacAddressesTest: mac address uniqueness test failed Changes: https://git.openjdk.java.net/jdk/pull/1076/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1076&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8246741 Stats: 83 lines in 1 file changed: 16 ins; 39 del; 28 mod Patch: https://git.openjdk.java.net/jdk/pull/1076.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1076/head:pull/1076 PR: https://git.openjdk.java.net/jdk/pull/1076
Re: RFR: 8246741: NetworkInterface/UniqueMacAddressesTest: mac address uniqueness test failed
On Thu, 5 Nov 2020 12:46:08 GMT, Conor Cleary wrote: > The primary goal of this fix was to refactor > NetworkInterface/UniqueMacAddressesTest.java to make use of the > jdk.test.lib.NetworkConfiguration class instead of java.net.NetworkInterface. > This was due to the original failure being related to the use of the 'awdl' > interface on macOS, which should be skipped for functional tests. The > NetworkConfiguration class accounts for this when returning the list of > present interfaces (originally retrieved using > NetworkInterface.getNetworkInterfaces()) as well as for other OS specific > properties. The effects of these changes can be seen on L59 and L101-106. > > In addition to these changes, some modifications were made to the test in the > interest of keeping it concise and readable but were made bearing in mind the > original logging of the test was already up to a good standard (and I sought > to keep it that way). To summarize the other changes: > > - When using NetworkConfiguration::interfaces to read in a stream of > NetworkInterface objects, the objects are collected into a list of records > (see [JEP 395](https://openjdk.java.net/jeps/395)) to avoid multiple calls to > NetworkInterface::getHardwareAddress (which can throw a SocketException) > throughout the test. > - When checking if the MAC addresses are unique, the testMacAddressesEqual() > method is not called unless the interface names are different. If > testMacAddressesEqual() returns true, the calling method will simply return > instead of updating the value of a boolean variable. > - Majority of logging was moved to the testMacAddressesEqualMethod() so that > only logs for comparisons are carried out. There is scope for additional > logging of, for example, the list of present interfaces returned by > createNetworkInterfaceList() if deemed necessary by review. LGTM. - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1076
Re: RFR: 8246741: NetworkInterface/UniqueMacAddressesTest: mac address uniqueness test failed
On Thu, 5 Nov 2020 13:59:18 GMT, Chris Hegarty wrote: >> The primary goal of this fix was to refactor >> NetworkInterface/UniqueMacAddressesTest.java to make use of the >> jdk.test.lib.NetworkConfiguration class instead of >> java.net.NetworkInterface. This was due to the original failure being >> related to the use of the 'awdl' interface on macOS, which should be skipped >> for functional tests. The NetworkConfiguration class accounts for this when >> returning the list of present interfaces (originally retrieved using >> NetworkInterface.getNetworkInterfaces()) as well as for other OS specific >> properties. The effects of these changes can be seen on L59 and L101-106. >> >> In addition to these changes, some modifications were made to the test in >> the interest of keeping it concise and readable but were made bearing in >> mind the original logging of the test was already up to a good standard (and >> I sought to keep it that way). To summarize the other changes: >> >> - When using NetworkConfiguration::interfaces to read in a stream of >> NetworkInterface objects, the objects are collected into a list of records >> (see [JEP 395](https://openjdk.java.net/jeps/395)) to avoid multiple calls >> to NetworkInterface::getHardwareAddress (which can throw a SocketException) >> throughout the test. >> - When checking if the MAC addresses are unique, the testMacAddressesEqual() >> method is not called unless the interface names are different. If >> testMacAddressesEqual() returns true, the calling method will simply return >> instead of updating the value of a boolean variable. >> - Majority of logging was moved to the testMacAddressesEqualMethod() so that >> only logs for comparisons are carried out. There is scope for additional >> logging of, for example, the list of present interfaces returned by >> createNetworkInterfaceList() if deemed necessary by review. > > LGTM. Great work, Conor. The test looks much better! Did you consider using testNG? Might help you condense things a bit more - PR: https://git.openjdk.java.net/jdk/pull/1076
Re: RFR: 8246741: NetworkInterface/UniqueMacAddressesTest: mac address uniqueness test failed
On Thu, 5 Nov 2020 14:10:15 GMT, Patrick Concannon wrote: > Great work, Conor. The test looks much better! > > Did you consider using testNG? Might help you condense things a bit more Thanks @pconcannon! I definitely do prefer testNG but was keen not to venture any further away from the scope of the fix than I may already have done with the other modifications. So with that in mind I tried to maintain the same test behavior i.e. throw a RuntimeException on failure instead of some sort of testNG assertion error. That being said, I am open to updating the test further to make use of testNG if additional feedback warrants it. - PR: https://git.openjdk.java.net/jdk/pull/1076
Re: RFR: 8246741: NetworkInterface/UniqueMacAddressesTest: mac address uniqueness test failed
On Thu, 5 Nov 2020 12:46:08 GMT, Conor Cleary wrote:
> The primary goal of this fix was to refactor
> NetworkInterface/UniqueMacAddressesTest.java to make use of the
> jdk.test.lib.NetworkConfiguration class instead of java.net.NetworkInterface.
> This was due to the original failure being related to the use of the 'awdl'
> interface on macOS, which should be skipped for functional tests. The
> NetworkConfiguration class accounts for this when returning the list of
> present interfaces (originally retrieved using
> NetworkInterface.getNetworkInterfaces()) as well as for other OS specific
> properties. The effects of these changes can be seen on L59 and L101-106.
>
> In addition to these changes, some modifications were made to the test in the
> interest of keeping it concise and readable but were made bearing in mind the
> original logging of the test was already up to a good standard (and I sought
> to keep it that way). To summarize the other changes:
>
> - When using NetworkConfiguration::interfaces to read in a stream of
> NetworkInterface objects, the objects are collected into a list of records
> (see [JEP 395](https://openjdk.java.net/jeps/395)) to avoid multiple calls to
> NetworkInterface::getHardwareAddress (which can throw a SocketException)
> throughout the test.
> - When checking if the MAC addresses are unique, the testMacAddressesEqual()
> method is not called unless the interface names are different. If
> testMacAddressesEqual() returns true, the calling method will simply return
> instead of updating the value of a boolean variable.
> - Majority of logging was moved to the testMacAddressesEqualMethod() so that
> only logs for comparisons are carried out. There is scope for additional
> logging of, for example, the list of present interfaces returned by
> createNetworkInterfaceList() if deemed necessary by review.
Changes requested by dfuchs (Reviewer).
test/jdk/java/net/NetworkInterface/UniqueMacAddressesTest.java line 38:
> 36: * @test
> 37: * @bug 8021372
> 38: * @summary Tests that the MAC addresses returned by
> NetworkConfiguration.probe() are unique for each adapter.
I would keep the summary as in the original test. We are really testing the
NetworkInterfaces returned by `NetworkInterface.getNetworkInterfaces` - even
though we are skipping some of them that we know are problematic.
test/jdk/java/net/NetworkInterface/UniqueMacAddressesTest.java line 108:
> 106: }
> 107:
> 108: record NetIfPair(String interfaceName, byte[] address) {}
Nit: I'd move that to line 46. It would avoid having to wonder what `NetIfPair`
is until you reach the end of the file.
-
PR: https://git.openjdk.java.net/jdk/pull/1076
Re: RFR: 8246741: NetworkInterface/UniqueMacAddressesTest: mac address uniqueness test failed [v2]
> The primary goal of this fix was to refactor > NetworkInterface/UniqueMacAddressesTest.java to make use of the > jdk.test.lib.NetworkConfiguration class instead of java.net.NetworkInterface. > This was due to the original failure being related to the use of the 'awdl' > interface on macOS, which should be skipped for functional tests. The > NetworkConfiguration class accounts for this when returning the list of > present interfaces (originally retrieved using > NetworkInterface.getNetworkInterfaces()) as well as for other OS specific > properties. The effects of these changes can be seen on L59 and L101-106. > > In addition to these changes, some modifications were made to the test in the > interest of keeping it concise and readable but were made bearing in mind the > original logging of the test was already up to a good standard (and I sought > to keep it that way). To summarize the other changes: > > - When using NetworkConfiguration::interfaces to read in a stream of > NetworkInterface objects, the objects are collected into a list of records > (see [JEP 395](https://openjdk.java.net/jeps/395)) to avoid multiple calls to > NetworkInterface::getHardwareAddress (which can throw a SocketException) > throughout the test. > - When checking if the MAC addresses are unique, the testMacAddressesEqual() > method is not called unless the interface names are different. If > testMacAddressesEqual() returns true, the calling method will simply return > instead of updating the value of a boolean variable. > - Majority of logging was moved to the testMacAddressesEqualMethod() so that > only logs for comparisons are carried out. There is scope for additional > logging of, for example, the list of present interfaces returned by > createNetworkInterfaceList() if deemed necessary by review. Conor Cleary has updated the pull request incrementally with one additional commit since the last revision: 8246741: Corrected summary tag, moved record declaration - Changes: - all: https://git.openjdk.java.net/jdk/pull/1076/files - new: https://git.openjdk.java.net/jdk/pull/1076/files/3e086804..1869acc6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1076&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1076&range=00-01 Stats: 4 lines in 1 file changed: 1 ins; 2 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1076.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1076/head:pull/1076 PR: https://git.openjdk.java.net/jdk/pull/1076
Re: RFR: 8252304: Seed an HttpRequest.Builder from an existing HttpRequest [v3]
On Wed, 4 Nov 2020 15:42:54 GMT, Daniel Fuchs wrote:
>> src/java.net.http/share/classes/java/net/http/HttpRequest.java line 344:
>>
>>> 342: throw ex;
>>> 343: } catch (RuntimeException r) {
>>> 344: throw new IllegalArgumentException("Illegal request
>>> parameters", r);
>>
>> I'm a little concerned about this. It seems unnecessary, and adds complexity
>> to an otherwise straightforward piece of code. Any accessor of the given
>> request that throws should probably just be allowed to flow out. If needed,
>> we could even mention that in the specification.
>
> The current code side step the issue of having to explain in the spec that
> any kind of `RuntimeException` could be propagated upwards. With this,
> calling code can just try catch `IAE`. It's a little more user-friendly.
Having discussed this offline, it was decided that we should remove the
try/catch altogether. This change can be viewed in commit
https://github.com/openjdk/jdk/pull/1059/commits/cabc0e700953058219cd0188faccdda12a7d0398
-
PR: https://git.openjdk.java.net/jdk/pull/1059
Re: RFR: 8252304: Seed an HttpRequest.Builder from an existing HttpRequest [v3]
On Wed, 4 Nov 2020 15:04:35 GMT, Chris Hegarty wrote:
>> Patrick Concannon 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 six additional
>> commits since the last revision:
>>
>> - 8252304: Removed catch block from newBuilder(HttpRequest)
>> - 8252304: assertBodyPublisherEqual added to test; added comment to
>> newBuilder
>> - Merge remote-tracking branch 'origin/master' into JDK-8252304
>> - Merge remote-tracking branch 'origin/master' into JDK-8252304
>> - Merge remote-tracking branch 'origin/master' into JDK-8252304
>> - 8252304: Seed an HttpRequest.Builder from an existing HttpRequest
>
> src/java.net.http/share/classes/java/net/http/HttpRequest.java line 317:
>
>> 315: * the given request (for instance, if the request contains
>> illegal
>> 316: * parameters)
>> 317: * @since TBD
>
> Please add the specific release number, in this case `16`.
Release number added in commit
https://github.com/openjdk/jdk/pull/1059/commits/7928083ae954c44b5357064b86af0e1a3f53588b
> test/jdk/java/net/httpclient/HttpRequestNewBuilderTest.java line 48:
>
>> 46: * @bug 8252304
>> 47: * @summary HttpRequest.NewBuilder(HttpRequest) API and behaviour checks
>> 48: * @compile --enable-preview -source ${jdk.version}
>> HttpRequestNewBuilderTest.java
>
> records are now final so these command line args can be removed.
Command line arguments removed in commit
https://github.com/openjdk/jdk/pull/1059/commits/7928083ae954c44b5357064b86af0e1a3f53588b
> src/java.net.http/share/classes/java/net/http/HttpRequest.java line 307:
>
>> 305:
>> 306: /**
>> 307: * Creates a {@code Builder} seeded from a {@code HttpRequest}.
>
> We use *an* HttpXXX consistently elsewhere. Please do the same here.
Typo corrected in commit
https://github.com/openjdk/jdk/pull/1059/commits/7928083ae954c44b5357064b86af0e1a3f53588b
-
PR: https://git.openjdk.java.net/jdk/pull/1059
Re: RFR: 8252304: Seed an HttpRequest.Builder from an existing HttpRequest [v3]
> Hi, > > Could someone please review our code for JDK-8252304: 'Seed an > HttpRequest.Builder from an existing HttpRequest'? > > This RFR proposes a new factory method for creating a new `HttpRequest` > builder from an existing `HttpRequest`. > This method can be used to build a new request equivalent to the given > request, but with different attributes. For instance, it will allow the user > to take an existing request and add or change a particular header, provide a > new `URI`, etc. > > > Kind regards, > Patrick & Chris Patrick Concannon 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 six additional commits since the last revision: - 8252304: Removed catch block from newBuilder(HttpRequest) - 8252304: assertBodyPublisherEqual added to test; added comment to newBuilder - Merge remote-tracking branch 'origin/master' into JDK-8252304 - Merge remote-tracking branch 'origin/master' into JDK-8252304 - Merge remote-tracking branch 'origin/master' into JDK-8252304 - 8252304: Seed an HttpRequest.Builder from an existing HttpRequest - Changes: - all: https://git.openjdk.java.net/jdk/pull/1059/files - new: https://git.openjdk.java.net/jdk/pull/1059/files/060aafae..cabc0e70 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1059&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1059&range=01-02 Stats: 182 lines in 11 files changed: 104 ins; 27 del; 51 mod Patch: https://git.openjdk.java.net/jdk/pull/1059.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1059/head:pull/1059 PR: https://git.openjdk.java.net/jdk/pull/1059
Re: RFR: 8252304: Seed an HttpRequest.Builder from an existing HttpRequest [v3]
On Wed, 4 Nov 2020 15:11:50 GMT, Daniel Fuchs wrote:
>> Patrick Concannon 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 six additional
>> commits since the last revision:
>>
>> - 8252304: Removed catch block from newBuilder(HttpRequest)
>> - 8252304: assertBodyPublisherEqual added to test; added comment to
>> newBuilder
>> - Merge remote-tracking branch 'origin/master' into JDK-8252304
>> - Merge remote-tracking branch 'origin/master' into JDK-8252304
>> - Merge remote-tracking branch 'origin/master' into JDK-8252304
>> - 8252304: Seed an HttpRequest.Builder from an existing HttpRequest
>
> src/java.net.http/share/classes/java/net/http/HttpRequest.java line 317:
>
>> 315: * the given request (for instance, if the request contains
>> illegal
>> 316: * parameters)
>> 317: * @since TBD
>
> `@since 16`
>
> we can change it again if it doesn't make the cut
Release number added in commit
https://github.com/openjdk/jdk/pull/1059/commits/7928083ae954c44b5357064b86af0e1a3f53588b
> src/java.net.http/share/classes/java/net/http/HttpRequest.java line 339:
>
>> 337: }
>> 338: }
>> 339: );
>
> I know what this piece of code does, and why it is necessary, but it would be
> good to add some `//` comments above to explain what is going on for the
> benefice of future maintainers
Comment added to clarify ifPresentOrElese statment. See commit
https://github.com/openjdk/jdk/pull/1059/commits/7928083ae954c44b5357064b86af0e1a3f53588b
> test/jdk/java/net/httpclient/HttpRequestNewBuilderTest.java line 272:
>
>> 270:assertThrows(IAE, () -> HttpRequest.newBuilder(r).build());
>> 271:}
>> 272: }
>
> It is customary to have a new line at the end of files.
New line added at end of file. See commit
https://github.com/openjdk/jdk/pull/1059/commits/7928083ae954c44b5357064b86af0e1a3f53588b
> test/jdk/java/net/httpclient/HttpRequestNewBuilderTest.java line 63:
>
>> 61:new NamedAssertion("headers",(r1,r2) ->
>> assertEquals(r1.headers(), r2.headers())),
>> 62:new NamedAssertion("expectContinue", (r1,r2) ->
>> assertEquals(r1.expectContinue(), r2.expectContinue()))
>> 63:);
>
> There should be an assertion for request bodies as well. You could check the
> following:
>
> 1. if r1 has a body, r2 must have a body
> 2. if r1 has no body, then r2 must not have a body
> 3. if both r1 and r2 have a body, then the body type & content length should
> be identical
> 4. you could subscribe to each body publisher and verify that the set of
> bytes you eventually get are identical
Additional assertion added to check all 4 points raised see L109-142 in commit
https://github.com/openjdk/jdk/pull/1059/commits/7928083ae954c44b5357064b86af0e1a3f53588b
> test/jdk/java/net/httpclient/HttpRequestNewBuilderTest.java line 47:
>
>> 45: * @test
>> 46: * @bug 8252304
>> 47: * @summary HttpRequest.NewBuilder(HttpRequest) API and behaviour checks
>
> Nit: `HttpRequest.newBuilder`
Typo fixed. See
https://github.com/openjdk/jdk/pull/1059/commits/7928083ae954c44b5357064b86af0e1a3f53588b
-
PR: https://git.openjdk.java.net/jdk/pull/1059
Re: RFR: 8246741: NetworkInterface/UniqueMacAddressesTest: mac address uniqueness test failed [v2]
On Thu, 5 Nov 2020 16:18:11 GMT, Conor Cleary wrote:
>> The primary goal of this fix was to refactor
>> NetworkInterface/UniqueMacAddressesTest.java to make use of the
>> jdk.test.lib.NetworkConfiguration class instead of
>> java.net.NetworkInterface. This was due to the original failure being
>> related to the use of the 'awdl' interface on macOS, which should be skipped
>> for functional tests. The NetworkConfiguration class accounts for this when
>> returning the list of present interfaces (originally retrieved using
>> NetworkInterface.getNetworkInterfaces()) as well as for other OS specific
>> properties. The effects of these changes can be seen on L59 and L101-106.
>>
>> In addition to these changes, some modifications were made to the test in
>> the interest of keeping it concise and readable but were made bearing in
>> mind the original logging of the test was already up to a good standard (and
>> I sought to keep it that way). To summarize the other changes:
>>
>> - When using NetworkConfiguration::interfaces to read in a stream of
>> NetworkInterface objects, the objects are collected into a list of records
>> (see [JEP 395](https://openjdk.java.net/jeps/395)) to avoid multiple calls
>> to NetworkInterface::getHardwareAddress (which can throw a SocketException)
>> throughout the test.
>> - When checking if the MAC addresses are unique, the testMacAddressesEqual()
>> method is not called unless the interface names are different. If
>> testMacAddressesEqual() returns true, the calling method will simply return
>> instead of updating the value of a boolean variable.
>> - Majority of logging was moved to the testMacAddressesEqualMethod() so that
>> only logs for comparisons are carried out. There is scope for additional
>> logging of, for example, the list of present interfaces returned by
>> createNetworkInterfaceList() if deemed necessary by review.
>
> Conor Cleary has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8246741: Corrected summary tag, moved record declaration
Marked as reviewed by dfuchs (Reviewer).
test/jdk/java/net/NetworkInterface/UniqueMacAddressesTest.java line 46:
> 44:
> 45: static PrintStream log = System.err;
> 46: record NetIfPair(String interfaceName, byte[] address) {}
Nit: maybe add a blank line, and a comment:
static PrintStream log = System.err;
// A record pair (NetworkInterface::name,
NetworkInterface::hardwareAddress)
record NetIfPair(String interfaceName, byte[] address) {}
-
PR: https://git.openjdk.java.net/jdk/pull/1076
Re: RFR: 8252304: Seed an HttpRequest.Builder from an existing HttpRequest [v3]
On Thu, 5 Nov 2020 16:23:13 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review our code for JDK-8252304: 'Seed an >> HttpRequest.Builder from an existing HttpRequest'? >> >> This RFR proposes a new factory method for creating a new `HttpRequest` >> builder from an existing `HttpRequest`. >> This method can be used to build a new request equivalent to the given >> request, but with different attributes. For instance, it will allow the user >> to take an existing request and add or change a particular header, provide a >> new `URI`, etc. >> >> >> Kind regards, >> Patrick & Chris > > Patrick Concannon 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 six additional > commits since the last revision: > > - 8252304: Removed catch block from newBuilder(HttpRequest) > - 8252304: assertBodyPublisherEqual added to test; added comment to > newBuilder > - Merge remote-tracking branch 'origin/master' into JDK-8252304 > - Merge remote-tracking branch 'origin/master' into JDK-8252304 > - Merge remote-tracking branch 'origin/master' into JDK-8252304 > - 8252304: Seed an HttpRequest.Builder from an existing HttpRequest Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1059
Re: RFR: 8253005: Add `@throws IOException` in javadoc for `HttpEchange.sendResponseHeaders` [v6]
> Hi, > > Could someone please review my fix for JDK-8253005: 'Add `@throws > IOException` in javadoc for `HttpEchange.sendResponseHeaders`' ? > > The method `HttpEchange.sendResponseHeaders` throws an `IOException` but is > unspecified in its javadoc. This fix adds an `@throws IOException` to its > specification and a description of the conditions under which the exception > is thrown. > > Kind regards, > Patrick Patrick Concannon 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 ten additional commits since the last revision: - Merge remote-tracking branch 'origin/master' into JDK-8253005 - 8253005: Added test to verify `IOException` thrown when headers already sent - Merge remote-tracking branch 'origin/master' into JDK-8253005 - Merge remote-tracking branch 'origin/master' into JDK-8253005 - Merge remote-tracking branch 'origin/master' into JDK-8253005 - Merge remote-tracking branch 'origin/master' into JDK-8253005 - Merge remote-tracking branch 'origin/master' into JDK-8253005 - Merge remote-tracking branch 'origin/master' into JDK-8253005 - 8253005: Add `@throws IOException` in javadoc for `HttpEchange.sendResponseHeaders` - Changes: - all: https://git.openjdk.java.net/jdk/pull/1014/files - new: https://git.openjdk.java.net/jdk/pull/1014/files/0c5c7744..f7363f7e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1014&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1014&range=04-05 Stats: 1770 lines in 166 files changed: 744 ins; 780 del; 246 mod Patch: https://git.openjdk.java.net/jdk/pull/1014.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1014/head:pull/1014 PR: https://git.openjdk.java.net/jdk/pull/1014
Re: RFR: 8246741: NetworkInterface/UniqueMacAddressesTest: mac address uniqueness test failed [v3]
> The primary goal of this fix was to refactor > NetworkInterface/UniqueMacAddressesTest.java to make use of the > jdk.test.lib.NetworkConfiguration class instead of java.net.NetworkInterface. > This was due to the original failure being related to the use of the 'awdl' > interface on macOS, which should be skipped for functional tests. The > NetworkConfiguration class accounts for this when returning the list of > present interfaces (originally retrieved using > NetworkInterface.getNetworkInterfaces()) as well as for other OS specific > properties. The effects of these changes can be seen on L59 and L101-106. > > In addition to these changes, some modifications were made to the test in the > interest of keeping it concise and readable but were made bearing in mind the > original logging of the test was already up to a good standard (and I sought > to keep it that way). To summarize the other changes: > > - When using NetworkConfiguration::interfaces to read in a stream of > NetworkInterface objects, the objects are collected into a list of records > (see [JEP 395](https://openjdk.java.net/jeps/395)) to avoid multiple calls to > NetworkInterface::getHardwareAddress (which can throw a SocketException) > throughout the test. > - When checking if the MAC addresses are unique, the testMacAddressesEqual() > method is not called unless the interface names are different. If > testMacAddressesEqual() returns true, the calling method will simply return > instead of updating the value of a boolean variable. > - Majority of logging was moved to the testMacAddressesEqualMethod() so that > only logs for comparisons are carried out. There is scope for additional > logging of, for example, the list of present interfaces returned by > createNetworkInterfaceList() if deemed necessary by review. Conor Cleary has updated the pull request incrementally with one additional commit since the last revision: 8246741: Added descriptive comment to record - Changes: - all: https://git.openjdk.java.net/jdk/pull/1076/files - new: https://git.openjdk.java.net/jdk/pull/1076/files/1869acc6..770455f3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1076&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1076&range=01-02 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/1076.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1076/head:pull/1076 PR: https://git.openjdk.java.net/jdk/pull/1076
Re: RFR: 8246741: NetworkInterface/UniqueMacAddressesTest: mac address uniqueness test failed [v2]
On Thu, 5 Nov 2020 16:30:31 GMT, Daniel Fuchs wrote:
>> Conor Cleary has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8246741: Corrected summary tag, moved record declaration
>
> test/jdk/java/net/NetworkInterface/UniqueMacAddressesTest.java line 46:
>
>> 44:
>> 45: static PrintStream log = System.err;
>> 46: record NetIfPair(String interfaceName, byte[] address) {}
>
> Nit: maybe add a blank line, and a comment:
> static PrintStream log = System.err;
>
> // A record pair (NetworkInterface::name,
> NetworkInterface::hardwareAddress)
> record NetIfPair(String interfaceName, byte[] address) {}
I agree, this is more clear for some one new looking at the test. Will add in
that comment now
-
PR: https://git.openjdk.java.net/jdk/pull/1076
Re: RFR: 8253005: Add `@throws IOException` in javadoc for `HttpEchange.sendResponseHeaders` [v5]
On Thu, 5 Nov 2020 10:52:19 GMT, Daniel Fuchs wrote:
>> Patrick Concannon 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 eight additional
>> commits since the last revision:
>>
>> - 8253005: Added test to verify `IOException` thrown when headers already
>> sent
>> - Merge remote-tracking branch 'origin/master' into JDK-8253005
>> - Merge remote-tracking branch 'origin/master' into JDK-8253005
>> - Merge remote-tracking branch 'origin/master' into JDK-8253005
>> - Merge remote-tracking branch 'origin/master' into JDK-8253005
>> - Merge remote-tracking branch 'origin/master' into JDK-8253005
>> - Merge remote-tracking branch 'origin/master' into JDK-8253005
>> - 8253005: Add `@throws IOException` in javadoc for
>> `HttpEchange.sendResponseHeaders`
>
> test/jdk/java/net/httpclient/SendResponseHeadersTest.java line 86:
>
>> 84: static class TestHandler implements HttpHandler {
>> 85: public void handle(HttpExchange exchange) throws IOException {
>> 86: exchange.sendResponseHeaders(200, 0);
>
> It's always preferable to read the request body fully before calling
> `sendResponseHeaders`
Thanks for pointing this out. I've added it to the test, and you can view it in
commit
https://github.com/openjdk/jdk/pull/1014/commits/814a7fd014d7706f63ed239f8be56ff26190b1f5
-
PR: https://git.openjdk.java.net/jdk/pull/1014
Re: RFR: 8253005: Add `@throws IOException` in javadoc for `HttpEchange.sendResponseHeaders` [v7]
> Hi, > > Could someone please review my fix for JDK-8253005: 'Add `@throws > IOException` in javadoc for `HttpEchange.sendResponseHeaders`' ? > > The method `HttpEchange.sendResponseHeaders` throws an `IOException` but is > unspecified in its javadoc. This fix adds an `@throws IOException` to its > specification and a description of the conditions under which the exception > is thrown. > > Kind regards, > Patrick Patrick Concannon has updated the pull request incrementally with one additional commit since the last revision: 8252304: Added read to TestHandler to ensure requestBody consumed before closing exchange - Changes: - all: https://git.openjdk.java.net/jdk/pull/1014/files - new: https://git.openjdk.java.net/jdk/pull/1014/files/f7363f7e..814a7fd0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1014&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1014&range=05-06 Stats: 14 lines in 1 file changed: 2 ins; 0 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/1014.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1014/head:pull/1014 PR: https://git.openjdk.java.net/jdk/pull/1014
Re: RFR: 8246741: NetworkInterface/UniqueMacAddressesTest: mac address uniqueness test failed [v3]
On Thu, 5 Nov 2020 17:01:07 GMT, Conor Cleary wrote: >> The primary goal of this fix was to refactor >> NetworkInterface/UniqueMacAddressesTest.java to make use of the >> jdk.test.lib.NetworkConfiguration class instead of >> java.net.NetworkInterface. This was due to the original failure being >> related to the use of the 'awdl' interface on macOS, which should be skipped >> for functional tests. The NetworkConfiguration class accounts for this when >> returning the list of present interfaces (originally retrieved using >> NetworkInterface.getNetworkInterfaces()) as well as for other OS specific >> properties. The effects of these changes can be seen on L59 and L101-106. >> >> In addition to these changes, some modifications were made to the test in >> the interest of keeping it concise and readable but were made bearing in >> mind the original logging of the test was already up to a good standard (and >> I sought to keep it that way). To summarize the other changes: >> >> - When using NetworkConfiguration::interfaces to read in a stream of >> NetworkInterface objects, the objects are collected into a list of records >> (see [JEP 395](https://openjdk.java.net/jeps/395)) to avoid multiple calls >> to NetworkInterface::getHardwareAddress (which can throw a SocketException) >> throughout the test. >> - When checking if the MAC addresses are unique, the testMacAddressesEqual() >> method is not called unless the interface names are different. If >> testMacAddressesEqual() returns true, the calling method will simply return >> instead of updating the value of a boolean variable. >> - Majority of logging was moved to the testMacAddressesEqualMethod() so that >> only logs for comparisons are carried out. There is scope for additional >> logging of, for example, the list of present interfaces returned by >> createNetworkInterfaceList() if deemed necessary by review. > > Conor Cleary has updated the pull request incrementally with one additional > commit since the last revision: > > 8246741: Added descriptive comment to record Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1076
Re: RFR: 8253005: Add `@throws IOException` in javadoc for `HttpEchange.sendResponseHeaders` [v7]
On Thu, 5 Nov 2020 17:08:10 GMT, Patrick Concannon
wrote:
>> Hi,
>>
>> Could someone please review my fix for JDK-8253005: 'Add `@throws
>> IOException` in javadoc for `HttpEchange.sendResponseHeaders`' ?
>>
>> The method `HttpEchange.sendResponseHeaders` throws an `IOException` but is
>> unspecified in its javadoc. This fix adds an `@throws IOException` to its
>> specification and a description of the conditions under which the exception
>> is thrown.
>>
>> Kind regards,
>> Patrick
>
> Patrick Concannon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8252304: Added read to TestHandler to ensure requestBody consumed before
> closing exchange
Changes requested by dfuchs (Reviewer).
test/jdk/java/net/httpclient/SendResponseHeadersTest.java line 99:
> 97: // unexpected exception thrown, return error to client
> 98: t.printStackTrace();
> 99: os.write(("Unexpected error: " + t).getBytes());
This should be:
os.write(("Unexpected error: " + t).getBytes(StandardCharsets.UTF_8));
test/jdk/java/net/httpclient/SendResponseHeadersTest.java line 94:
> 92: try {
> 93: IOException io = expectThrows(IOException.class,
> 94: () -> exchange.sendResponseHeaders(200,
> "failMsg".getBytes().length));
It would be clearer to use a constant value here: either 0, or some meaningless
value > 0 and < 16
-
PR: https://git.openjdk.java.net/jdk/pull/1014
Re: RFR: 8252304: Seed an HttpRequest.Builder from an existing HttpRequest [v3]
On Thu, 5 Nov 2020 16:23:13 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review our code for JDK-8252304: 'Seed an >> HttpRequest.Builder from an existing HttpRequest'? >> >> This RFR proposes a new factory method for creating a new `HttpRequest` >> builder from an existing `HttpRequest`. >> This method can be used to build a new request equivalent to the given >> request, but with different attributes. For instance, it will allow the user >> to take an existing request and add or change a particular header, provide a >> new `URI`, etc. >> >> >> Kind regards, >> Patrick & Chris > > Patrick Concannon 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 six additional > commits since the last revision: > > - 8252304: Removed catch block from newBuilder(HttpRequest) > - 8252304: assertBodyPublisherEqual added to test; added comment to > newBuilder > - Merge remote-tracking branch 'origin/master' into JDK-8252304 > - Merge remote-tracking branch 'origin/master' into JDK-8252304 > - Merge remote-tracking branch 'origin/master' into JDK-8252304 > - 8252304: Seed an HttpRequest.Builder from an existing HttpRequest Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1059
Re: RFR: 8253005: Add `@throws IOException` in javadoc for `HttpEchange.sendResponseHeaders` [v7]
On Thu, 5 Nov 2020 17:08:10 GMT, Patrick Concannon
wrote:
>> Hi,
>>
>> Could someone please review my fix for JDK-8253005: 'Add `@throws
>> IOException` in javadoc for `HttpEchange.sendResponseHeaders`' ?
>>
>> The method `HttpEchange.sendResponseHeaders` throws an `IOException` but is
>> unspecified in its javadoc. This fix adds an `@throws IOException` to its
>> specification and a description of the conditions under which the exception
>> is thrown.
>>
>> Kind regards,
>> Patrick
>
> Patrick Concannon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8252304: Added read to TestHandler to ensure requestBody consumed before
> closing exchange
Changes requested by chegar (Reviewer).
test/jdk/java/net/httpclient/SendResponseHeadersTest.java line 63:
> 61: ExecutorService executor = Executors.newCachedThreadPool();
> 62: server.setExecutor (executor);
> 63: server.start();
Being a little pedantic, then the setup and teardown of the server could be
moved into an `@BeforeTest` and `@AfterTest`, which would leave the client part
of the test uncluttered.
test/jdk/java/net/httpclient/SendResponseHeadersTest.java line 69:
> 67: server.getAddress().getPort(),
> 68: path, null, null);
> 69:
This maybe fine, but the pattern we've used elsewhere ( and have proven to be
reliable ) is: InetAddress.getLoopbackAddress().getHostName() and
server.getAddress().getPort(), but now I think that maybe these could be an
issue for IPv6-only hosts?
test/jdk/java/net/httpclient/SendResponseHeadersTest.java line 75:
> 73: .GET()
> 74: .build();
> 75: HttpResponse response = client.send(request,
> HttpResponse.BodyHandlers.ofString());
How about we import java.net.http.HttpResponse.BodyHandlers?
test/jdk/java/net/httpclient/SendResponseHeadersTest.java line 99:
> 97: // unexpected exception thrown, return error to client
> 98: t.printStackTrace();
> 99: os.write(("Unexpected error: " + t).getBytes());
Hmm... I'd be tempted to drop this catch block altogether, it's not possible to
know exactly what to do there? given that we don't know what the failure is.
-
PR: https://git.openjdk.java.net/jdk/pull/1014
Re: RFR: 8253005: Add `@throws IOException` in javadoc for `HttpEchange.sendResponseHeaders` [v7]
On Thu, 5 Nov 2020 17:35:09 GMT, Chris Hegarty wrote:
>> Patrick Concannon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8252304: Added read to TestHandler to ensure requestBody consumed before
>> closing exchange
>
> test/jdk/java/net/httpclient/SendResponseHeadersTest.java line 99:
>
>> 97: // unexpected exception thrown, return error to
>> client
>> 98: t.printStackTrace();
>> 99: os.write(("Unexpected error: " + t).getBytes());
>
> Hmm... I'd be tempted to drop this catch block altogether, it's not possible
> to know exactly what to do there? given that we don't know what the failure
> is.
This is executed on the server side - the Throwable here is the AssertionError
thrown by expectThrows if the expected exception is not thrown. It is important
to return this to the client side so that the client side (main thread) can
fail properly.
-
PR: https://git.openjdk.java.net/jdk/pull/1014
