RE: RFR (XS): 8169865: Downport minor fixes in java.net native code from JDK 9 to JDK 8

[Langer, Christoph]

Thanks, Chris.

It's pushed: http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/rev/90d2c6ddcedd

> -Original Message-
> From: Chris Hegarty [mailto:chris.hega...@oracle.com]
> Sent: Montag, 21. November 2016 11:54
> To: Langer, Christoph 
> Cc: net-dev@openjdk.java.net; jdk8u-...@openjdk.java.net
> Subject: Re: RFR (XS): 8169865: Downport minor fixes in java.net native code
> from JDK 9 to JDK 8
> 
> On 17/11/16 12:12, Langer, Christoph wrote:
> > Hi,
> >
> > please review a downport of a few small fixes to JDK 8 which were done
> > through bugs 8034174, 8034182 and 8048518 to JDK 9 already.
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8169865
> >
> > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8169865.8udev/
> 
> The changes look fine.
> 
> -Chris
> 
> >
> > The change in Inet6AddressImpl.c is required because AIX would currently
> > use the Solaris coding which is not desired. The changes in
> > net_util_md.c fix a potential resource leak and some other code spot was
> > changed where our code base currently has an unnecessary diff.
> >
> > Thanks for reviewing.
> >
> > Best regards
> > Christoph

RE: RFR(M): 8167420: remove redundant code path in unix/native/libnet/Inet4AddressImpl.c

[Langer, Christoph]

Hi Chris,

thanks for looking into this.

First of all I'm glad that you agree to make the getLocalHostname call 
consistent throughout all implementations. :)

However, my feeling at the moment rather is to prefer just return the 
gethostname() API result instead of doing getaddrinfo/getnameinfo on top. Is 
there anything speaking against that? Maybe a Java property could be introduced 
to enable getaddrinfo/getnameinfo in case some scenario would need that?

Best regards
Christoph



> -Original Message-
> From: Chris Hegarty [mailto:chris.hega...@oracle.com]
> Sent: Montag, 21. November 2016 15:18
> To: Langer, Christoph ; net-dev@openjdk.java.net
> Subject: Re: RFR(M): 8167420: remove redundant code path in
> unix/native/libnet/Inet4AddressImpl.c
> 
> Hi Christoph,
> 
> On 03/11/16 15:46, Langer, Christoph wrote:
> > Hi again,
> >
> > I have to make one addition to my points:
> >
> > Java_java_net_Inet6AddressImpl_getLocalHostName:
> >
> > -  made getaddrinfo/getnameinfo turnaround the default, before
> > it was only used on solaris. But it should be the default since
> > Inet4Addressimpl does it as well?? Or do we want to remove it completely??
> 
> I'd say make it the default, which is what you have in your webrev.
> It seems mostly redundant on platforms that return the FQDN, but
> being consistent would be better.
> 
> > For this one I just had a customer case where I really had a hard time
> > to figure out why on one of his Linux machines InetAddres.getLocaHost()
> > would return an FQDN and on the other it wouldn't. It eventually turned
> > out that on the machine where FQDN was returned, no IPv4 addresses were
> > configured and hence he used the Inet4AddressImpl version to do the
> > getaddrinfo/getnameinfo to lookup the FQDN. On his other machine, where
> > IPv6 is configured, the Inet6AddressImpl variant is used and we only get
> > the short name. So I really think this should be aligned, shouldn't it?
> 
> Yes.
> 
> > Maybe it's really better to always return what the gethostname API says.
> > Or is there a reason why IPv4 and IPv6 handling is different?
> 
> I cannot find any specific reason why these would be different.
> 
> -Chris.
> 
> > Best regards
> >
> > Christoph
> >
> >
> >
> > *From:*Langer, Christoph
> > *Sent:* Mittwoch, 2. November 2016 15:25
> > *To:* 'net-dev@openjdk.java.net' 
> > *Subject:* RFR(M): 8167420: remove redundant code path in
> > unix/native/libnet/Inet4AddressImpl.c
> >
> >
> >
> > Hi,
> >
> >
> >
> > I respun my proposal for cleaning up Inet4AddressImpl.c and
> > Inet6AddressImpl.c:
> >
> > http://cr.openjdk.java.net/~clanger/webrevs/8167420.1/
> >
> >
> >
> > So, apart from dropping the implementation for the rare AllBSD/MacOS
> > code, I suggest the following things:
> >
> >
> >
> > Java_java_net_Inet4AddressImpl_getLocalHostName:
> >
> > -  rare mac version used AF_UNSPEC for getaddrinfo call, new
> > version uses AF_INET which is probably more correct
> >
> >
> >
> > Java_java_net_Inet4AddressImpl_lookupAllHostAddr:
> >
> > - remove isspace(hostname[0]) check (solaris and strange mac version had
> > it). It should check if the hostname starts with a blank and throw an
> > UnknownHostException in that case. However, my testing on current
> > Solaris and MacOS versions shows me that it is not needed and the
> > UnknownHostException is thrown anyway.
> >
> > - standard version now has the MacOS fallback "lookupIfLocalhost" (which
> > only strange mac version had before). This function lookupifLocalhost is
> > called if getnameinfo returns with an error and localhost addresses
> > shall be determined. Then getaddrinfo would be used to enumerate local
> > addresses. However, I would generally question that call - or we could
> > as well have an implementation of that fallback based on the
> > NetworkInterface class. Any oppinions?
> >
> >
> >
> > Java_java_net_Inet6AddressImpl_getLocalHostName:
> >
> > -  made getaddrinfo/getnameinfo turnaround the default, before
> > it was only used on solaris. But it should be the default since
> > Inet4Addressimpl does it as well?? Or do we want to remove it completely??
> >
> >
> >
> > Java_java_net_Inet6AddressImpl_lookupAllHostAddr:
> >
> > -  remove isspace check (for solaris), same reasons as with
> > Java_java_net_Inet4AddressImpl_lookupAllHostAddr
> >
> >
> >
> > Java_java_net_Inet6AddressImpl_getHostByAddr:
> >
> > -  replace CHECK_NULL_RETURN(ret, NULL) with
> > UnknownHostException in case of getnameinfo error. This seems more
> > correct if you look at the usage of getHostByAddr in InetAddress.java
> >
> >
> >
> > My local tests did not show any problems. I'll add the fix to the patch
> > queues in our internal OpenJDK build environment to see if problems
> > appear. I would appreciate any feedback on the points I elaborated above.
> >
> >
> >
> > Thanks & Best regards
> >
> > Christoph
> >
> >
> >
> >
> >
> > *From:*Langer, Christoph
> > *Sent:* Montag, 10. Oktober

Re: HTTP client API

[Chris Hegarty]

Tobias,

If you look at the code in the sandbox [*], the notion of a default
client has been removed. Having global static default instances is
problematic. Http Clients are lightweight, easy to configure and
pass around, if that is what you want.

-Chris.

[*] hg clone http://hg.openjdk.java.net/jdk9/sandbox; cd sandbox
bash common/bin/hgforest.sh update http-client-branch


On 21/11/16 11:28, Tobias Thierer wrote:

Replying to my own first email in this thread to add another question /
concern about flexibility of the HTTP Client API:

Have you considered offering applications a way to globally replace the
HTTP Client implementation with their own (e.g. via a method
HttpClient.setDefault() to go with the existing method
HttpClient.getDefault())?
This feature seems to currently be missing from the API while even the
old HttpURLConnection API supported this (via
URL.setURLStreamHandlerFactory()
).

I'm aware that such global (process-wide) configuration options have
disadvantages as well, but it would be consistent with other swappable
Java APIs/SPIs such as SSLSocketFactory, XML parsers, CookieHandler,
etc.  The advantage would be that applications / application frameworks
could swap out the HttpClient implementation used by lower level
libraries / applications that they're bundling in binary form - e.g. to
globally provide a fake implementation to use during testing, instrument
HTTP Client usage (e.g. log all URLs accessed or count the number of
bytes transferred), to adhere to requirements in some corporate
networks, or to otherwise decouple the version/implementation of the
HTTP Client from the platform / OpenJDK version. What's your view?

Tobias


On Mon, Oct 24, 2016 at 8:33 PM, Tobias Thierer mailto:tobi...@google.com>> wrote:

Hi Michael and others -


Thanks for publishing the latest HTTP client API docs
(already
slightly outdated again), as well as for publishing the current
draft code in the sandbox repository!


Below is some concrete feedback, questions and brainstorming on how to

(a) increase the usefulness or

(b) decrease the semantic weight

of the API. Note that most of this is driven only by inspection of
the API and some brief exploration of the implementation code, not
(yet) by a substantial effort to write proof of concept client
applications. I’d love if I could help make this API as useful to
applications as possible, so I’d appreciate your feedback on how I
can best do that and what the principles were that guided your
design choices.


1.) The HttpRequest.BodyProcessor

and
HttpResponse.BodyProcessor

abstractions
seem particularly hard to grasp / have a high semantics weight.

  *

What purpose does the abstraction of a BodyProcessoraim to
fulfill beyond what the (simpler) abstraction of a Bodycould be?

  o

Instead of describing the abstraction as a “processor” of
ByteBuffers / Java objects, wouldn’t it be simpler to say to
say that request / response bodiesare ByteBuffer / Java
object sources/ sinks? What is the advantage of the
Publisher / Subscriber API over
plain old InputStream / OutputStream based APIs?

  o

The term “processor” and the description of “converting
incoming buffers of data to some user-defined object type T”
is especially confusing (increases the semantic weight of
the abstraction) given that there is an implementation that
discards all data

(and
its generic type is called Urather than T). A BodyProcessor
that has no input but generates the digits of Pi is also
conceivable. Perhaps call these BodySource / BodySink,
ByteBufferPublisher / ByteBufferSubscriber, or just Body?

  o

The fact that you felt the need to introduce an abstraction
HttpResponse.BodyHandler whose name is similar to but whose
semantics are different from HttpResponse.BodyProcessor is
another indication that these concepts could be clarified
and named better.

  o

To explore how well the abstractions “fit”, I played with
some draft code implementing the API on top of another one;
one thing I found particularly challenging was the control
flow progression:
HttpClient.sen

Re: RFR(M): 8167420: remove redundant code path in unix/native/libnet/Inet4AddressImpl.c

[Chris Hegarty]

Hi Christoph,

On 03/11/16 15:46, Langer, Christoph wrote:

Hi again,

I have to make one addition to my points:

Java_java_net_Inet6AddressImpl_getLocalHostName:

-  made getaddrinfo/getnameinfo turnaround the default, before
it was only used on solaris. But it should be the default since
Inet4Addressimpl does it as well?? Or do we want to remove it completely??


I'd say make it the default, which is what you have in your webrev.
It seems mostly redundant on platforms that return the FQDN, but
being consistent would be better.


For this one I just had a customer case where I really had a hard time
to figure out why on one of his Linux machines InetAddres.getLocaHost()
would return an FQDN and on the other it wouldn’t. It eventually turned
out that on the machine where FQDN was returned, no IPv4 addresses were
configured and hence he used the Inet4AddressImpl version to do the
getaddrinfo/getnameinfo to lookup the FQDN. On his other machine, where
IPv6 is configured, the Inet6AddressImpl variant is used and we only get
the short name. So I really think this should be aligned, shouldn’t it?


Yes.


Maybe it’s really better to always return what the gethostname API says.
Or is there a reason why IPv4 and IPv6 handling is different?


I cannot find any specific reason why these would be different.

-Chris.


Best regards

Christoph



*From:*Langer, Christoph
*Sent:* Mittwoch, 2. November 2016 15:25
*To:* 'net-dev@openjdk.java.net' 
*Subject:* RFR(M): 8167420: remove redundant code path in
unix/native/libnet/Inet4AddressImpl.c



Hi,



I respun my proposal for cleaning up Inet4AddressImpl.c and
Inet6AddressImpl.c:

http://cr.openjdk.java.net/~clanger/webrevs/8167420.1/



So, apart from dropping the implementation for the rare AllBSD/MacOS
code, I suggest the following things:



Java_java_net_Inet4AddressImpl_getLocalHostName:

-  rare mac version used AF_UNSPEC for getaddrinfo call, new
version uses AF_INET which is probably more correct



Java_java_net_Inet4AddressImpl_lookupAllHostAddr:

- remove isspace(hostname[0]) check (solaris and strange mac version had
it). It should check if the hostname starts with a blank and throw an
UnknownHostException in that case. However, my testing on current
Solaris and MacOS versions shows me that it is not needed and the
UnknownHostException is thrown anyway.

- standard version now has the MacOS fallback “lookupIfLocalhost” (which
only strange mac version had before). This function lookupifLocalhost is
called if getnameinfo returns with an error and localhost addresses
shall be determined. Then getaddrinfo would be used to enumerate local
addresses. However, I would generally question that call – or we could
as well have an implementation of that fallback based on the
NetworkInterface class. Any oppinions?



Java_java_net_Inet6AddressImpl_getLocalHostName:

-  made getaddrinfo/getnameinfo turnaround the default, before
it was only used on solaris. But it should be the default since
Inet4Addressimpl does it as well?? Or do we want to remove it completely??



Java_java_net_Inet6AddressImpl_lookupAllHostAddr:

-  remove isspace check (for solaris), same reasons as with
Java_java_net_Inet4AddressImpl_lookupAllHostAddr



Java_java_net_Inet6AddressImpl_getHostByAddr:

-  replace CHECK_NULL_RETURN(ret, NULL) with
UnknownHostException in case of getnameinfo error. This seems more
correct if you look at the usage of getHostByAddr in InetAddress.java



My local tests did not show any problems. I’ll add the fix to the patch
queues in our internal OpenJDK build environment to see if problems
appear. I would appreciate any feedback on the points I elaborated above.



Thanks & Best regards

Christoph





*From:*Langer, Christoph
*Sent:* Montag, 10. Oktober 2016 09:32
*To:* 'net-dev@openjdk.java.net' mailto:net-dev@openjdk.java.net>>
*Subject:* RFR(S): 8167420: remove redundant code path in
unix/native/libnet/Inet4AddressImpl.c



Hi,



here’s another review request for a cleanup:



Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8167420.0/

Bug: https://bugs.openjdk.java.net/browse/JDK-8167420



In unix/native/libnet/Inet4AddressImpl.c, there exist 2 implementations
for each of Java_java_net_Inet4AddressImpl_getLocalHostName,
Java_java_net_Inet4AddressImpl_lookupAllHostAddr and
Java_java_net_Inet4AddressImpl_getHostByAddr. I think one branch is
obsolete.



I also did some cleanups in those functions. One question that I still
have is if we should add the MACOSX workaround path when getaddrinfo
returns error and “lookupIfLocalhost” is called? However, as it was not
part of the standard branch which is probably used mostly on MacOSX
nowadays, I tend to remove it. In the webrev it is still contained.



The changeset is based on my proposal for 8167295

(http://cr.openjdk.java.net/~clanger/webrevs/8167295.1/).



Thanks & Best regards

Christoph

Re: HTTP client API

[Tobias Thierer]

Hi Michael -

Some follow-up/updates on items from my previous email inline below:

On Mon, Oct 31, 2016 at 6:13 PM, Tobias Thierer  wrote:

>
> On Fri, Oct 28, 2016 at 12:28 PM, Michael McMahon <
> michael.x.mcma...@oracle.com> wrote:
>
>>
>> The intention behind those interfaces is primarily to provide a
>> conversion between ByteBuffers and higher level objects that users
>> are interested in. So, HttpRequest.BodyProcessor generates ByteBuffers
>> for output and HttpResponse.ByteBuffer consumes them
>> on input. For response bodies, there is this additional implicit
>> requirement of needing to do something with the incoming data
>> and that is why the option of discarding it exists (the details of that
>> API are certainly open to discussion)
>>
>> On Publisher/Subscriber (Flow) versus InputStream/OutputStream, we wanted
>> a non blocking API
>>
>
> I can understand the desire to have a non blocking API, especially if
> performance is seen as more important for the API than usability. Cronet
> (the Chromium networking stack) API [documentation
> ]
> has a
>
> UrlRequest.read(ByteBuffer) <-> callback.onReadCompleted(UrlRequest,
> UrlResponseInfo, ByteBuffer)
>
> call cycle that is not too different from OpenJDK 9 HTTP client's
>
>subscription.request(1) <-> BodyProcessor.onNext(ByteBuffer)
>
> so the general idea is shared by others (I'm meeting with some Cronet
> folks later this week so I hope to gather their input / views soon).
>

I've meanwhile met with Cronet folks and now agree with your view that an
async API is useful for applications that prioritize performance over
simplicity. That said, I still think that many applications will prefer the
simplicity of a blocking API - either provided as part of the platform or
as a separate library.

FYI as an experiment to see how easy this could be, I've prototyped a
blocking API on top of the async API, specifically an adapter from OpenJDK
9's HttpResponse.BodyProcessor to an InputStream.

The way I've prototyped it was to use a PipedInputStream internally whose
buffer size is 32KBytes (2 * the size of the ByteBuffers used to deliver
the response); the number of bytes free in that internal buffer

   (a) decreases when onResponseBodyChunk() copies bytes into the
associated PipedOutputStream,
   (b) increases when the application reads bytes from the PipedInputStream.

The PipedInputStream is wrapped in another InputStream that calls
flowController.accept(1) every time (b) frees up enough bytes in the
internal buffer to fit everything that may be delivered in the next call to
onResponseBodyChunk(); this ensures that onResponseBodyChunk() never
blocks. The InputStream wrapper methods also take care of throwing
IOException after onResponseError(Throwable) occurred.

All of this seemed fairly straightforward, but I did run into some problems:

   - I haven't succeeded in building OpenJDK 9 from source, following the
   instructions (I always run into a NullPointerException in XMLDTDProcessor
   during the BUILD_TOOLS_CORBA part of "make all"), so I've been unable to
   empirically try / benchmark my prototype code.
   - Obviously, copying data into/out of the PipedInputStream's internal
   buffer is overhead; this consciously trades performance for simplicity of
   the API. (I note that ResponseContent.copyBuffer() also performs copying
   but I haven't checked if this can be avoided).
   - flowController.accept(1) is only called when the underlying
   PipedInputStream.read() returns, and then there will still be additional
   latency before more data is delivered to onResponseBodyChunk(). These
   delays will impair performance more than should ideally be required but I
   haven't been able to quantify this through benchmarks because of my
   problems of building OpenJDK 9 from source.
  - Further contributing to this latency is the fact that the wrapper
  doesn't have good control over how many bytes will be delivered
during the
  next onResponseBodyChunk(). Note that I've run into this same
limitation on
  my previous prototype for the use case of parsing image data.
Further note
  that Cronet's API *does* give the application control over how many
  bytes at most will be delivered during the next
onReadCompleted(UrlRequest,
  UrlResponseInfo, ByteBuffer) - see my previous email. I
previously thought
  that it was the BodyProcessor's job to construct the ByteBuffer
objects but
  I may be mistaken - in the latest version of the API, this seems to be an
  implementation detail of HttpClientImpl and not part of the public API.

On the question of push versus pull, there seems to be differing
>> philosophies around this, but
>> actually the publish/subscribe model provided by the Flow types has
>> characteristics of both push and pull.
>> It is primarily push() as in Subscriber.onNext() delivers the next buffer
>> to the subscriber. But, th

Re: RFR (XS): 8169865: Downport minor fixes in java.net native code from JDK 9 to JDK 8

[Chris Hegarty]

On 17/11/16 12:12, Langer, Christoph wrote:

Hi,

please review a downport of a few small fixes to JDK 8 which were done
through bugs 8034174, 8034182 and 8048518 to JDK 9 already.

Bug: https://bugs.openjdk.java.net/browse/JDK-8169865

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8169865.8udev/


The changes look fine.

-Chris



The change in Inet6AddressImpl.c is required because AIX would currently
use the Solaris coding which is not desired. The changes in
net_util_md.c fix a potential resource leak and some other code spot was
changed where our code base currently has an unnecessary diff.

Thanks for reviewing.

Best regards
Christoph