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
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 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: RFE: Add PATCH to the list of the supported HTTP methods
Hi Chris, > The JDK HTTP Client has: > `HttpRequest.Builder::method(String method, BodyPublisher publisher)` > > ,so it is currently possible to use the `PATCH` method. > > Is `PATCH` sufficiently popular to warrant its own self-named > method in the request builder? I don't know how popular is it. We have just started to use it with one of our services and discovered that we cannot use it with HttpURLConnection at all. And I thought when we add the support for PATCH to HttpURLConnection, then it would be nice to have it in HTTP Client too. When you decided not to add it to HTTP Client, then it is OK for me too. But I think that HttpURLConnection should be changed. Best regards, Andrej Golovnin
RFE: Add PATCH to the list of the supported HTTP methods
Hi all, it would be nice if could add PATCH (https://tools.ietf.org/html/rfc5789) to the list of the supported HTTP methods in the HttpURLConnection class. I have attached two patches: - one for the HttpURLConnection class: java_net_HttpURLConnection_PATCH_method.diff - and one for the HTTP Client branch in the JDK sandbox repository which adds PATCH to the HttpRequest builder: java_net_http_HttpRequest_PATCH_method.diff Best regards, Andrej Golovnin java_net_HttpURLConnection_PATCH_method.diff Description: Binary data java_net_http_HttpRequest_PATCH_method.diff Description: Binary data
Re: RFR JDK-8087113: Websocket API and implementation
> On Sun, Apr 10, 2016 at 2:00 PM, Andrej Golovnin > <andrej.golov...@gmail.com> wrote: >> BTW, someone should describe in the JavaDocs of CompletableFuture#orTimeout() >> what would happen when this method is called multiple times on the same >> CompletableFuture with different arguments. > > Hmmm the javadoc seems clear enough to me. CF can only be completed > once. First to complete wins. What is the first: the first call to #orTimeout() or the call with the lowest timeout? CompletableFuture cf = ... cf.onTimeout(10L, TimeUnit.MINUTES); // 1. call cf.onTimeout(10L, TimeUnit.SECONDS); // 2. call cf.onTimeout(10L, TimeUnit.HOURS); // 3. call As far as I understand the implementation, the second call wins. What I miss from the JavaDocs is following: It is allowed to call the #onTimeout()-methods multiple times. But it does not mean that subsequent calls would override already defined timeout and that the lowest timeout wins.
Re: RFR JDK-8087113: Websocket API and implementation
n
you can change this method to:
onClose(WebSocket webSocket, Optional closeReason)
I think it makes the API much better and it is clear that the reason phrase is
only
available when you have a close code too.
And I think there should be a way to configure a timeout for sending messages.
One minor thing off-topic:
While changing my code I noticed that the class
ProxySelector$StaticProxySelector
and the JavaDocs of the ProxySelector#of-method can be improved a little bit:
diff -r 589795e4cd38 src/java.base/share/classes/java/net/ProxySelector.java
--- a/src/java.base/share/classes/java/net/ProxySelector.java Wed Mar 23
19:33:39 2016 -0700
+++ b/src/java.base/share/classes/java/net/ProxySelector.java Mon Mar 28
19:30:24 2016 +0200
@@ -165,7 +165,8 @@
/**
* Returns a ProxySelector which uses the given proxy address for all HTTP
- * and HTTPS requests. If proxy is {@code null} then proxying is disabled.
+ * and HTTPS requests. If the given proxy address is {@code null}
+ * then proxying is disabled.
*
* @param proxyAddress
*The address of the proxy
@@ -178,9 +179,9 @@
return new StaticProxySelector(proxyAddress);
}
-static class StaticProxySelector extends ProxySelector {
+private static final class StaticProxySelector extends ProxySelector {
private static final List NO_PROXY_LIST =
List.of(Proxy.NO_PROXY);
-final List list;
+private final List list;
StaticProxySelector(InetSocketAddress address){
Proxy p;
@@ -198,9 +199,9 @@
}
@Override
-public synchronized List select(URI uri) {
-String scheme = uri.getScheme().toLowerCase();
-if (scheme.equals("http") || scheme.equals("https")) {
+public List select(URI uri) {
+String scheme = uri.getScheme();
+if (scheme.equalsIgnoreCase("http") ||
scheme.equalsIgnoreCase("https")) {
return list;
} else {
return NO_PROXY_LIST;
Best regards,
Andrej Golovnin
Re: WebSocket client API
Hi Pavel, the JavaDocs for the method HttpClient.Builder#proxy(java.net.ProxySelector selector) say: Sets a ProxySelector for this client. If no selector is set, then no proxies are used. If a null parameter is supplied then the system wide default proxy selector is used. Really? For me it means that nearly every application which would like to use the new HttpClient/WebSocket must call this method with null value. Just think about Java WebStart applications and applets: you never know if there is a proxy or not. IMHO the JavaDocs for this method should look like this: Sets a ProxySelector for this client. If no proxy selector is set, then the system wide default proxy selector is used. @throws IllegalArgumentException if selector is null. I'm sure the most developers out there should be able to provide their own implementation of the java.net.ProxySelector class which just returns Collections.singletonList(Proxy.NO_PROXY) in the method ProxySelector#select(URI uri) if they need direct access to a server. And if you would like to simplify the life of the developers which need a direct access to a server, then you could add a new method to the builder: HttpClient.Builder#noProxy() I think it is better than to add a special meaning to the null value. And I think that the phrase with "If no proxy selector is set, then the system wide default proxy selector is used." should be a part of the JavaDocs of the class HttpClient.Builder and the method HttpClient.getDefault() to make clear the behaviour of the created/returned HttpClient. Best regards, Andrej Golovnin
Re: WebSocket client API
Hi Chris, >>> Will adding the ability to send ping(ByteBuffer) be sufficient for your >>> usage? If so, then I think we should add it, but not pong. The >>> implementation will automatically send a pong message in response to >>> receiving a ping. OK? >> >> Yes, ping(ByteBuffer) is sufficient for us. > > I assume you will also need a way to distinguish that the incoming data is > from a pong message, in response to your ping, right? I'm not sure what you mean. If you mean, that we need to distinguish it in the method WebSocket.Incoming.onBinary(WebSocket, ByteBuffer), then no. I expect that this method is only used to process binary message. For pong messages I would expect a new method: WebSocket.Incoming.onPong(WebSocket, ByteBuffer). Currently we don't process the pong messages on the client. And there are no plans to do it in the near future. Best regards, Andrej Golovnin
Re: WebSocket client API
Hi Pavel, >> - WebSocket.onClose() should contain the close reason code. > > What is it useful for? Is it ok to provide it as a part of description? > > String description = char code + ": " + String reason > Please don't do that! We definitely don't want to parse it. What we need is a method with this signature: WebSocket.Incoming.onClose(int statusCode, String reason) In our application an administrator may close a connection due to server maintenance or some other reason and in this case we use an application specific statusCode (e.g. 4001). Depending on the status code we show the user appropriate messages and the reason submitted by the administrator. What we also need, is the method WebSocket.close(int statusCode, String reason) because the client may also close the connection for some application specific reason. Best regards, Andrej Golovnin
Re: WebSocket client API
Hi Michael,
> Can you explain why you need j.n.Proxy rather than the
> String/InetSocketAddress
> combination proposed for HttpClient?
String is not type safe. And allowing to define proxy only for the
specific protocol is not enough from my experience. Sometimes you have
to choose proxy based on the host of the URL you try to connect to.
What HttpClient.Builder.proxy() should really take as parameter, is
the j.n.ProxySelector. The most code, I have seen so far, does not
really care about proxies. But everyone expects that it still works
with proxies. j.n.URL.openConnection() gives us this magic because at
some point in sun.net.www.protocol.http.HttpURLConnection the method
ProxySelector.getDefault().select(URI) is called for the URL and the
connection is established using the proxy. The same behaviour I expect
from HttpClient and WebSocket. Therefore I think you have to deal with
j.n.Proxy and j.n.ProxySelector in the HttpClient code anyway. And
that's why I think you should use them in the Public API of HttpClient
and WebSocket as well.
Here is a small summary of the API I would like to see:
/**
* Defines a selector to be used by this client to select the proxy
server for a request.
* If no selector is defined for this client, the client will use
{@link j.n.ProxySelector.getDefault()} as it's selector.
*
* @param selectorthe selector to be used by this client; may not be null.
*
* @throws IllegalArgumentException if selector is null.
*/
j.n.HttpClient.Builder.proxySelector(j.n.ProxySelector selector)
/**
* Defines the proxy to be used by this request.
* If no proxy is defined for this request, the selector from the
HttpClient will be used to select the proxy.
*
* @param proxy the proxy to be used by this request; may not be null.
*
* @throws IllegalArgumentException if proxy is null.
*/
j.n.HttpRequest.Builder.proxy(j.n.Proxy proxy)
/**
* Defines the proxy to be used by this WebSocket.
* If no proxy is defined for this WebSocket, the default selector
{@link j.n.ProxySelector.getDefault()} will be used to select the
proxy.
*
* @param proxy the proxy to be used by this WebSocket; may not be null.
*
* @throws IllegalArgumentException if proxy is null.
*/
j.n.WebSocket.Builder.proxy(j.n.Proxy proxy)
As always fell free to use it or to ignore it. My code is already in
production and works. :-)
I hope this helps.
Best regards,
Andrej Golovnin
Re: WebSocket client API
Hi Pavel, > So you need the ability to send pings and unsolicited pongs. Do you handle > pong > replies from the server? If so, do you control their arrival times and their > consistency with previous pings sent? What about unsolicited pongs from the > server? We use pings to ensure that firewalls do not close inactive (from their point of view) connections. And we send some useful data with the ping which is then processed on the server. Currently all pong messages from the server are ignored on the client. Best regards, Andrej Golovnin
Re: WebSocket client API
Hi Chris, > Will adding the ability to send ping(ByteBuffer) be sufficient for your > usage? If so, then I think we should add it, but not pong. The > implementation will automatically send a pong message in response to > receiving a ping. OK? Yes, ping(ByteBuffer) is sufficient for us. Best regards, Andrej Golovnin
Re: WebSocket client API
Hi Pavel, >> - Where's the .ping() or .pong() ? > > * @apiNote Keep-alive features of WebSocket protocol are taken care of > * completely by implementations and are not exposed in this API. > > We thought that a high-level API could live without this burden for the user. > At > the same time the implementation will definitely have several configuration > parameters, tweaking its behaviour in respect to keep-alive features. > > Yes, we've thought about application data that can be carried by those types > of > messages, but due to restrictions to their size (125 bytes) and potential > out-of-order delivery we've decided not to expose them as data carriers. Just because you don’t see usages for it, it does not mean that others don’t need it. We have an application which already make use of it. And if you don’t add at least the #ping(ByteBuffer) method, it would not be usable for us and we will stay with JSR 356. What I also miss, is the ability to define custom HTTP header values which should be transmitted during the handshake. And I think you should add an API to define the proxy in case some one use just WebSocket.newBuilder(String). And please use the class java.net.Proxy to define a proxy and not just a String and a InetSocketAddress as it is done in HttpClient. Best regards, Andrej Golovnin
