java.net.http.HttpClient: invalid exception when bad status line is returned
Hello,
I am tasting java.net.http.HttpClient with
openjdk version "11.0.1" 2018-10-16
OpenJDK Runtime Environment AdoptOpenJDK (build 11.0.1+13)
OpenJDK 64-Bit Server VM AdoptOpenJDK (build 11.0.1+13, mixed mode)
on Mac OS X 10.14.2.
Consider the case when server responds with bad (invalid format) status line:
"HTTP/1.1 FOO"
The following IllegalArgumentException is thrown:
Exception in thread "main" java.lang.IllegalArgumentException: For input
string: "FOO"
at
java.net.http/jdk.internal.net.http.HttpClientImpl.send(HttpClientImpl.java:551)
at
java.net.http/jdk.internal.net.http.HttpClientFacade.send(HttpClientFacade.java:119)
at ru.mitya.test.App.main(App.java:14)
Caused by: java.lang.NumberFormatException: For input string: "FOO"
at
java.base/java.lang.NumberFormatException.forInputString(NumberFormatException.java:65)
at java.base/java.lang.Integer.parseInt(Integer.java:652)
at java.base/java.lang.Integer.parseInt(Integer.java:770)
at
java.net.http/jdk.internal.net.http.Http1HeaderParser.readStatusLineFeed(Http1HeaderParser.java:197)
at
java.net.http/jdk.internal.net.http.Http1HeaderParser.parse(Http1HeaderParser.java:124)
at
java.net.http/jdk.internal.net.http.Http1Response$HeadersReader.handle(Http1Response.java:672)
at
java.net.http/jdk.internal.net.http.Http1Response$HeadersReader.handle(Http1Response.java:603)
at
java.net.http/jdk.internal.net.http.Http1Response$Receiver.accept(Http1Response.java:594)
at
java.net.http/jdk.internal.net.http.Http1Response$HeadersReader.tryAsyncReceive(Http1Response.java:650)
at
java.net.http/jdk.internal.net.http.Http1AsyncReceiver.flush(Http1AsyncReceiver.java:228)
at
java.net.http/jdk.internal.net.http.common.SequentialScheduler$SynchronizedRestartableTask.run(SequentialScheduler.java:175)
at
java.net.http/jdk.internal.net.http.common.SequentialScheduler$CompleteRestartableTask.run(SequentialScheduler.java:147)
at
java.net.http/jdk.internal.net.http.common.SequentialScheduler$SchedulableTask.run(SequentialScheduler.java:198)
at
java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
at
java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
at java.base/java.lang.Thread.run(Thread.java:834)
instead of the expected ProtocolException:
Exception in thread "main" java.io.IOException: Invalid status line: "HTTP/1.1
FOO"
at
java.net.http/jdk.internal.net.http.HttpClientImpl.send(HttpClientImpl.java:565)
at
java.net.http/jdk.internal.net.http.HttpClientFacade.send(HttpClientFacade.java:119)
at ru.mitya.test.App.main(App.java:14)
Caused by: java.net.ProtocolException: Invalid status line: "HTTP/1.1 FOO"
<...>
The suggested fix would be to catch IllegalArgumentException thrown by
Integer.parseInt() in readStatusLineFeed() and throw
ProtocolException instead of it.
What do you think?
Thanks.
Here is a sample test program:
package ru.mitya.test;
import java.net.URI;
import java.net.http.HttpClient;
import java.net.http.HttpRequest;
import java.net.http.HttpResponse;
public class App {
public static void main(String[] args) throws Exception {
HttpClient client = HttpClient.newBuilder().build();
HttpRequest req =
HttpRequest.newBuilder(URI.create("http://localhost:8000/";))
.version(HttpClient.Version.HTTP_1_1)
.build();
HttpResponse resp = client.send(req,
HttpResponse.BodyHandlers.ofString());
}
}
Re: [13] RFR: 8216561: HttpClient: The logic of retry on connect exception is inverted
Hi Daniel, > Right - here is a better implementation anyway: > http://cr.openjdk.java.net/~dfuchs/webrev_8216561/webrev.02/ > Looks much better. Thanks a lot! Best regards, Andrej Golovnin
Re: [13] RFR: 8216561: HttpClient: The logic of retry on connect exception is inverted
On 18/01/2019 14:24, Daniel Fuchs wrote: Hi Andrej, On 18/01/2019 14:03, Andrej Golovnin wrote: Is creating new Optionals a real problem? And before you answer please remember what Donald Knuth said: The real problem is that programmers have spent far too much time worrying about efficiency in the wrong places and at the wrong times; premature optimization is the root of all evil (or at least most of it) in programming. ;-) Right - here is a better implementation anyway: http://cr.openjdk.java.net/~dfuchs/webrev_8216561/webrev.02/ This looks good Daniel. -Chris.
Re: [13] RFR: 8216561: HttpClient: The logic of retry on connect exception is inverted
Hi Andrej, On 18/01/2019 14:03, Andrej Golovnin wrote: Is creating new Optionals a real problem? And before you answer please remember what Donald Knuth said: The real problem is that programmers have spent far too much time worrying about efficiency in the wrong places and at the wrong times; premature optimization is the root of all evil (or at least most of it) in programming. ;-) Right - here is a better implementation anyway: http://cr.openjdk.java.net/~dfuchs/webrev_8216561/webrev.02/ best regards, -- daniel Best regards and have a nice weekend, Andrej Golovnin
Re: RFR: 8207404: MulticastSocket tests failing on AIX
On 18/01/2019 08:54, Langer, Christoph wrote: Hi, here is the updated webrev with modifications to the problem list and reverting the static imports: _http://cr.openjdk.java.net/~clanger/webrevs/8207404.1/__ I think this is ok. -Chris.
Re: [13] RFR: 8216561: HttpClient: The logic of retry on connect exception is inverted
Hi Daniel, Yes that looks like my first implementation. But then > I reflected that avoiding to map Optional to Duration > and then back to a new Optional containing the same > duration could be avoided by simply storing the original > optional obtained from the HttpClient. > > The current code only creates a new instance of Optional > when it needs to. > Is creating new Optionals a real problem? And before you answer please remember what Donald Knuth said: The real problem is that programmers have spent far too much time worrying about efficiency in the wrong places and at the wrong times; premature optimization is the root of all evil (or at least most of it) in programming. ;-) Best regards and have a nice weekend, Andrej Golovnin
Re: [13] RFR: 8216561: HttpClient: The logic of retry on connect exception is inverted
Hi Andrej,
Yes that looks like my first implementation. But then
I reflected that avoiding to map Optional to Duration
and then back to a new Optional containing the same
duration could be avoided by simply storing the original
optional obtained from the HttpClient.
The current code only creates a new instance of Optional
when it needs to.
best regards,
-- daniel
On 18/01/2019 12:42, Andrej Golovnin wrote:
Hi Daniel,
webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8216561/webrev.01/
126 private static class ConnectTimeoutTracker {
127 final Optional max;
128 final AtomicLong startTime = new AtomicLong();
129 ConnectTimeoutTracker(Optional connectTimeout) {
130 this.max = connectTimeout;
131 }
132
133 Optional getRemaining() {
134 if (max.isEmpty()) return max;
135 long now = System.nanoTime();
136 long previous = startTime.compareAndExchange(0, now);
137 if (previous == 0 || max.get().isZero()) return max;
138 Duration remaining =
max.get().minus(Duration.ofNanos(now - previous));
139 assert remaining.compareTo(max.get()) <= 0;
140 return Optional.of(remaining.isNegative() ?
Duration.ZERO : remaining);
141 }
142
143 void reset() { startTime.set(0); }
144 }
An Optional in a class field or in a data structure, is considered a
misuse of the API (see the explanation by Stuart Marks:
https://stackoverflow.com/questions/23454952/uses-for-optional/23464794#23464794).
I would write it this way:
private static class ConnectTimeoutTracker {
final Duration max;
final AtomicLong startTime = new AtomicLong();
ConnectTimeoutTracker(Duration connectTimeout) {
this.max = connectTimeout;
}
Optional getRemaining() {
return Optional.ofNullable(max)
.map(m -> {
long now = System.nanoTime();
long previous = startTime.compareAndExchange(0, now);
if (previous == 0 || m.isZero()) {
return m;
}
Duration remaining = m.minus(Duration.ofNanos(now
- previous));
assert remaining.compareTo(m) <= 0;
return remaining.isNegative() ? Duration.ZERO :
remaining;
});
}
void reset() { startTime.set(0); }
}
Best regards,
Andrej Golovnin
RE: RFR: 8207404: MulticastSocket tests failing on AIX
Hi Christoph, Had a look at the latest webrev and everything looks OK to me. Have also re-tested on my AIX systems and all tests oass OK. Thanks Steve Groeger IBM Runtime Technologies Hursley, Winchester Tel: (44) 1962 816911 Mobex: 279990 Mobile: 07718 517 129 Fax (44) 1962 816800 Lotus Notes: Steve Groeger/UK/IBM Internet: groe...@uk.ibm.com Unless stated otherwise above: IBM United Kingdom Limited - Registered in England and Wales with number 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU From: "Langer, Christoph" To: Chris Hegarty , Steve Groeger Cc: "net-dev@openjdk.java.net" , "ppc-aix-port-...@openjdk.java.net" Date: 18/01/2019 08:54 Subject:RE: RFR: 8207404: MulticastSocket tests failing on AIX Hi, here is the updated webrev with modifications to the problem list and reverting the static imports: http://cr.openjdk.java.net/~clanger/webrevs/8207404.1/ It went fine through our test system so I think it’s good now. Please review. Thanks Christoph From: Chris Hegarty Sent: Donnerstag, 17. Januar 2019 12:27 To: Langer, Christoph Cc: net-dev@openjdk.java.net; ppc-aix-port-...@openjdk.java.net Subject: Re: RFR: 8207404: MulticastSocket tests failing on AIX On 17 Jan 2019, at 10:55, Langer, Christoph wrote: Hi, here is a fix for the MulticastSocket tests failing on AIX (in certain sceanrios): Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8207404.0/ Why remove the static imports from NetworkConfiguration? It makes the code more verbose ( and IMO harder to read ). -Chris. Unless stated otherwise above: IBM United Kingdom Limited - Registered in England and Wales with number 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
Re: [13] RFR: 8216561: HttpClient: The logic of retry on connect exception is inverted
Hi Daniel,
> webrev:
> http://cr.openjdk.java.net/~dfuchs/webrev_8216561/webrev.01/
>
>
>
126 private static class ConnectTimeoutTracker {
127 final Optional max;
128 final AtomicLong startTime = new AtomicLong();
129 ConnectTimeoutTracker(Optional connectTimeout) {
130 this.max = connectTimeout;
131 }
132
133 Optional getRemaining() {
134 if (max.isEmpty()) return max;
135 long now = System.nanoTime();
136 long previous = startTime.compareAndExchange(0, now);
137 if (previous == 0 || max.get().isZero()) return max;
138 Duration remaining = max.get().minus(Duration.ofNanos(now
- previous));
139 assert remaining.compareTo(max.get()) <= 0;
140 return Optional.of(remaining.isNegative() ? Duration.ZERO
: remaining);
141 }
142
143 void reset() { startTime.set(0); }
144 }
An Optional in a class field or in a data structure, is considered a misuse
of the API (see the explanation by Stuart Marks:
https://stackoverflow.com/questions/23454952/uses-for-optional/23464794#23464794).
I would write it this way:
private static class ConnectTimeoutTracker {
final Duration max;
final AtomicLong startTime = new AtomicLong();
ConnectTimeoutTracker(Duration connectTimeout) {
this.max = connectTimeout;
}
Optional getRemaining() {
return Optional.ofNullable(max)
.map(m -> {
long now = System.nanoTime();
long previous = startTime.compareAndExchange(0, now);
if (previous == 0 || m.isZero()) {
return m;
}
Duration remaining = m.minus(Duration.ofNanos(now -
previous));
assert remaining.compareTo(m) <= 0;
return remaining.isNegative() ? Duration.ZERO :
remaining;
});
}
void reset() { startTime.set(0); }
}
Best regards,
Andrej Golovnin
[13] RFR: 8216561: HttpClient: The logic of retry on connect exception is inverted
Hi, Please find below a small fix for: 8216561: HttpClient: The logic of retry on connect exception is inverted https://bugs.openjdk.java.net/browse/JDK-8216561 webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8216561/webrev.01/ The patch now allows retry on connect exception, ensuring that the second attempt takes into account the time spent in the first attempt in order to honor the connect timeout value (if present). best regards, -- daniel
RE: RFR: 8207404: MulticastSocket tests failing on AIX
Hi, here is the updated webrev with modifications to the problem list and reverting the static imports: http://cr.openjdk.java.net/~clanger/webrevs/8207404.1/ It went fine through our test system so I think it's good now. Please review. Thanks Christoph From: Chris Hegarty Sent: Donnerstag, 17. Januar 2019 12:27 To: Langer, Christoph Cc: net-dev@openjdk.java.net; ppc-aix-port-...@openjdk.java.net Subject: Re: RFR: 8207404: MulticastSocket tests failing on AIX On 17 Jan 2019, at 10:55, Langer, Christoph mailto:christoph.lan...@sap.com>> wrote: Hi, here is a fix for the MulticastSocket tests failing on AIX (in certain sceanrios): Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8207404.0/ Why remove the static imports from NetworkConfiguration? It makes the code more verbose ( and IMO harder to read ). -Chris.
