Re: Suggestion for the HTTP client: allow selection of a local address

2018-08-08 Thread Michael McMahon

Klaus

I've created an RFE at https://bugs.openjdk.java.net/browse/JDK-8209137
to track this. I imagine the simplest solution would be to be able to 
specify the local
address when constructing a HttpClient, and then all connections created 
using that

client would be bound to the chosen address.

Michael.

On 08/08/2018, 15:43, Klaus Malorny wrote:



Hi,

I just discussed with a colleague that we dreadfully miss the ability 
in the old HttpURLConnection to set the address to which the client 
locally binds, which we require to get through the firewalls of our 
business partners when connecting from a multihomed system. This 
repeatedly caused us to use third-party libraries, which are powerful 
on the one hand, but cumbersome on the other.


With the new HTTP2 client in mind, I wanted to tell him, wait, with 
Java 9+, it will be better. But I checked first and discovered that 
there is still no way to do so.


So, when you eventually revise the API, it would be nice if you could 
consider the addition of such a feature.


Regards,

Klaus



Suggestion for the HTTP client: allow selection of a local address

2018-08-08 Thread Klaus Malorny




Hi,

I just discussed with a colleague that we dreadfully miss the ability in the old 
HttpURLConnection to set the address to which the client locally binds, which we 
require to get through the firewalls of our business partners when connecting 
from a multihomed system. This repeatedly caused us to use third-party 
libraries, which are powerful on the one hand, but cumbersome on the other.


With the new HTTP2 client in mind, I wanted to tell him, wait, with Java 9+, it 
will be better. But I checked first and discovered that there is still no way to 
do so.


So, when you eventually revise the API, it would be nice if you could consider 
the addition of such a feature.


Regards,

Klaus



Re: Priority can be set for HttpClient with HTTP/1.1

2018-08-08 Thread Chris Hegarty
Hi Nezih,

The client’s version is a default preferred version for requests that do not 
specify an explicit version. As such, it does not prevent requests, attempting 
to negotiate HTTP/2, from being sent, just that it is not the default. Given 
that, then the priority value is still be applicable to client’s build with a 
preferred HTTP/1.1 version.

-Chris.

> On 1 Aug 2018, at 10:56, nezih yigitbasi  wrote:
> 
> Hi,
> Priority is only defined for version HTTP_2 (and that's clear from the 
> javadoc). However, it's possible to construct an instance of HttpClient with 
> a priority value and with version HTTP_1_1 (tested with Java "9.0.1"). I feel 
> that it will be less error prone for the users if conflicting combinations of 
> arguments weren't allowed. It's also confusing when I read the below code as 
> it doesn't make sense to set the priority for HTTP/1.1. What do you think?
> 
> HttpClient client = HttpClient.newBuilder()
> .executor(httpClientExecutor)
> .priority(32)
> .version(HTTP_1_1)
> .build();
> 
> Thanks,
> Nezih



Re: [httpclient] HTTP2: Memory Leak with Proxy

2018-08-08 Thread Albert Schimpf

Chris,

that's good to hear, I'll try it out.

Best,
Albert

On 07.08.2018 18:53, Chris Hegarty wrote:

Albert,

I haven’t yet looked at what happens in JDK 10, but just to say, since things 
have moved on a lot in JDK 11 EA, that the same test runs with a reasonable 
amount of memory and CPU with JDK 11 EA [1]. I updated the test a little, since 
some the names have been changed with the standardisation ( see javadoc for 
more details [2] ).

-Chris.

[1] http://jdk.java.net/11/
[2] 
https://download.java.net/java/early_access/jdk11/docs/api/java.net.http/java/net/http/package-summary.html

---

$ cat BadProxyLeak.java

import java.net.InetSocketAddress;
import java.net.ProxySelector;
import java.net.URI;
import java.net.http.HttpClient;
import java.net.http.HttpRequest;
import java.net.http.HttpResponse;
import java.util.concurrent.TimeUnit;

public class BadProxyLeak {
 public static void main(String[] args) throws InterruptedException {
 // 1st heap dump
//Thread.sleep(1);

 {
 HttpClient client = HttpClient.newBuilder()
 // malicious proxy?
 .proxy(ProxySelector.of(new 
InetSocketAddress("165.165.248.90", 8080)))
 .build();

 HttpRequest req = HttpRequest
 // target is not relevant
 .newBuilder(URI.create("https://www.google.de";))
 // leak occurs only with HTTP_2
 .version(HttpClient.Version.HTTP_2)
//.version(HttpClient.Version.HTTP_1_1)
 .GET()
 .build();


 // use same client for every request
 // most likely happens on the first retry, if not try again or 
increase i
 for (int i = 0; i < 10; i++) {
 System.out.println("Request " + i);
 // body handler is not relevant
 HttpResponse.BodyHandler t = 
HttpResponse.BodyHandlers.ofString();

 try {
 // happens with both async and sync send
 client.sendAsync(req, t).get(15, TimeUnit.SECONDS);
//client.send(req, handler);
 } catch (Exception e) {
 e.printStackTrace();
 }
 }
 }

 // 3rd heap dump
 System.out.println("Generating heap dump manually");
 // space is not released
 Thread.sleep(6);
 }
}



On 7 Aug 2018, at 17:05, Chris Hegarty  wrote:



On 7 Aug 2018, at 16:55, Albert Schimpf  wrote:

Hi,

by bad I mean that this proxy is the only one out of ~1000 proxies which causes 
this behavior. It's also the only one which causes SSLExceptions (General SSL 
engine problem) and EOFExceptions. I don't think that particular proxy is a 
valid proxy. Using curl indicates that this is a SSL handshake problem (https):

* Rebuilt URL to: www.google.de/
*   Trying 165.165.248.90…

Oh, you mean the proxy at that actual IP address. Ok got it.



* TCP_NODELAY set
* Connected to 165.165.248.90 (165.165.248.90) port 8080 (#0)
* successfully set certificate verify locations:
*   CAfile: /etc/ssl/certs/ca-certificates.crt
  CApath: /etc/ssl/certs
* TLSv1.2 (OUT), TLS handshake, Client hello (1):
* OpenSSL SSL_connect: SSL_ERROR_SYSCALL in connection to 165.165.248.90:8080
* Closing connection 0
curl: (35) OpenSSL SSL_connect: SSL_ERROR_SYSCALL in connection to 
165.165.248.90:8080

I don't know that much about the protocol itself,  so I don't think I can help 
very much.

Thank you for looking into this!


-Chris.



Best,
Albert


On 07.08.2018 12:05, Chris Hegarty wrote:

Hi Albert,

Very strange indeed. Thanks for reporting it, I’ll investigate.

What do your mean by “bad proxy”. What is bad about it, and how does it behave?

-Chris.



On 7 Aug 2018, at 10:25, Albert Schimpf  wrote:

Hi,

I stumbled upon some strange behavior when using the new Java httpclient.

The issue is very simple to reproduce. Send a GET request via a known bad proxy:

HttpClient client = HttpClient.newBuilder()
.proxy(ProxySelector.of(BAD_PROXY))
.build();

HttpRequest req = HttpRequest
// target is not relevant
.newBuilder(...)
.GET()
.build();

// body handler is not relevant
HttpResponse.BodyHandler t = HttpResponse.BodyHandler.asString();

// happens with both async and sync send
client.sendAsync(req, t).get(30, TimeUnit.SECONDS);

The result is that the heap size increases dramatically (to about 1.5GB) and 
resources are not released. CPU consumption increases by a constant factor, 
too. I have tried many variations of the above code, and the only thing which 
seems to work (i.e. heap size does not explode) is to set the HTTP version to 
1.1.

In my main application this leads to both memory and CPU starvation (4GB memory 
limit, 100% CPU usage). It us

Re: RFR [11] 8208391: Need to differentiate response and connect timeouts in HTTP Client API

2018-08-08 Thread Michael McMahon

The updated webrev looks fine to me, Chris.

Thanks,
Michael

On 07/08/2018, 10:52, Chris Hegarty wrote:

On 4 Aug 2018, at 14:08, Jaikiran Pai  wrote:

...

Do you think this can be reworded a bit? Although I understand what's
being said here, the wording doesn't seem right. Maybe something like:

  *  In the case where a new connection needs to be
established, if
  * the connection cannot be established within the given {@code
  * duration}, then {@link HttpClient#send(java.net.http.HttpRequest,
  * java.net.http.HttpResponse.BodyHandler) HttpClient::send}
throws a
  * {@link HttpConnectTimeoutException}, or
  * {@link HttpClient#sendAsync(java.net.http.HttpRequest,
  * java.net.http.HttpResponse.BodyHandler) HttpClient::sendAsync}
  * completes exceptionally with an {@code
HttpConnectTimeoutException}.

Agreed. This wording avoids the previous awkwardness.


src/java.net.http/share/classes/jdk/internal/net/http/HttpRequestImpl.java

-this.timeout = null;
+this.timeout = Duration.ofSeconds(30);

Is this an intentional change of default value for the timeout? Is that
something that needs to be documented?

Accidental, left over from a previous hacking session. Removed.

Updated webrev:
   http://cr.openjdk.java.net/~chegar/8208391/webrev.01/


One other thing - maybe not directly related to this single patch, but
as you are aware, recently as part of [1], a new system property (and a
security property) was introduced to optionally include host info in the
exception messages thrown for socket exceptions

I am aware of this.


. Does the HttpClient
honour that system property in the exceptions it throws?

The HTPT Client does not have any special handing for this property.
It may not be necessary, at least not for low-level NIO exceptions, once
the exception, or its cause, is preserved.


I don't see it
being used in this patch for the timeout exceptions. I haven't checked
the code, outside of this patch, to see if it's dealt with in other
parts of this API.

  Separately, I will look into the possibility of where such an extension
can be used.

-Chris.