Hi Jaikiran,
A couple of remarks:
> However, in this specific case the Builder is expected
> to be created using the static java.net.http.HTTPClient.newBuilder()
> method and that implementation always returns an internal implementation
> of the Builder interface. That effectively means that the only
> implementation impacted by this new method addition would be the JDK
> internal jdk.internal.net.http.HttpClientBuilderImpl. Because of this, I
> decided not to create this above new method as a "default" one.
I do not think this is right. Even though the only implementation
is in the JDK, libraries that provides adds-on on top of the JDK API
could very well provide their own builder implementation - obtained
by other means than calling HttpClient::newBuilder().
In this case I would recommend having a default body that
throws UnsupportedOperationException - and adding the @throws
to the API documentation of the new method.
You will also need an @implSpec to document the default body,
and possibly say that builders obtained from HttpClient::newBuilder
provide an implementation of this method.
There will also be some security checks that need to be performed
probably in the builder implementation, either when passing the
address or possibly when build() is called (or maybe both?).
See SocketChannel::bind. @throws SecurityException with appropriate
documentation will need to be added where needed.
The HttpClient does these checks before hand - so I believe a
security check needs to happen in build() at the latest.
There's also the question on whether we should take a SocketAddress
here, even though at this time we only accept InetAddress.
There might be pro and cons.
> In addition to the above method on the Builder interface, a new method
> has also been introduced on the java.net.http.HttpClient class:
>
> /**
> * Returns an {@link InetAddress} which will be used by this client
> * to bind a socket when creating connections. If no
> * {@link Builder#localAddress() local address} was set in the
> * client's builder, then this method returns null, in which case
> * the sockets created by this client will be bound to automatically
> * assigned socket addresses.
> *
> * @return the local address that will be used by this client when
> * creating connections
> * @since 19
> */
> public InetAddress localAddress() {
> return null;
> }
I am not sure we should add that.
If we do add this method, then it should probably
return Optional<InetAddress> like other methods in this
class (e.g. proxy(), authenticator(), selector())
WRT to SocketChannel::bind I believe we should treat that as a non
blocking call - as it shouldn't involve any network operation.
Finally, supplying a local address to the HttpClient builder is really
for advanced users. Only hosts reachable through that interface will
be connectable. This means that providing a "bad" address there
can prevent accessing the servers targetted by the request URI.
In pathological cases it could also prevent accessing the proxy
returned by the proxy selector.
The documentation of Builder::localAddress should probably have
more information about the use cases for supplying a local address
and some warning about the consequences of doing so.
Michael may have more insight/comments to provide.
best regards,
-- daniel
On 03/12/2021 06:21, Jaikiran Pai wrote:
I've been working on adding support for the enhancement noted in
https://bugs.openjdk.java.net/browse/JDK-8209137. As noted in that
issue, right now the java.net.http.HTTPClient doesn't have a way to
allow applications to specify which local address to use while
connecting to target hosts during requests >
The proposed enhancement will introduce an API to allow applications to
configure a particular local address that the HTTPClient will use when
creating Socket(s) during connection. I've an initial version of this
enhancement that I've created as a draft PR here
https://github.com/openjdk/jdk/pull/6690 for initial discussion. I would
like some inputs on these changes.
User facing API:
---------------
A new API has been introduced on the java.net.http.HTTPClient.Builder
interface:
/**
* Binds the socket to this local address when creating
* connections for sending requests.
*
* <p> If no local address is set or {@code null} is passed
* to this method then sockets created by the
* HTTP client will be bound to an automatically
* assigned socket address.
*
* @param localAddr The local socket address. Can be null.
* @return this builder
* @since 19
*/
public Builder localAddress(InetAddress localAddr);
The java.net.http.HTTPClient.Builder is a public API and typically any
new method additions to such interfaces would mean using a "default"
method so as to prevent and breakages of existing implementations of
that interface. However, in this specific case the Builder is expected
to be created using the static java.net.http.HTTPClient.newBuilder()
method and that implementation always returns an internal implementation
of the Builder interface. That effectively means that the only
implementation impacted by this new method addition would be the JDK
internal jdk.internal.net.http.HttpClientBuilderImpl. Because of this, I
decided not to create this above new method as a "default" one.
This new locaAddress method accepts an instance of java.net.InetAddress.
Calling applications are expected to create the InetAddress using
different available methods. I paid some thought to see if it's worth
allowing calling applications to pass a "hostname/IP" String as a
parameter to this new method instead of accepting a InetAddress, but
decided to expect a pre-constructed InetAddress to keep it simple (for
the HTTPClient) as well as to let the application/callers deal with the
InetAddress creation.
Calling this method is optional and as noted in the javadoc of this
method, passing null or not calling this method will have the same
result - the connections created by the HTTPClient will end up using an
automatically assigned socket address, just like it does in the current
version.
Furthermore, the Builder, neither in the implementation of this method
nor during the build() call will do any validations on the passed
InetAddress. In other words, the localAddress that's set on the
HTTPClient will just be used as-is during connection creation and any
failures to bind due to the passed InetAddress will be propagated back
as a IOException. I spent some thought on whether to do any kind of
validations, like whether the host of the InetAddress is resolvable and
such, during the Builder.build() but decided not to. One reason for that
is, the HTTPClient is a long lived object and, as far as I remember, is
encouraged to be used as one single instance for the lifetime of an
application. The address resolution result of an InetAddress (as per its
javadoc) are cachable values and are refreshed from time to time (based
on internal implementation details of that class). As such, there's no
guarantee that a "valid" InetAddress during Builder.build() would still
be "valid" when the connection is being established by the HTTPClient at
some later point in time (maybe some hours later). So doing such
validations at Builder.build() time, I thought, would add no real value.
In addition to the above method on the Builder interface, a new method
has also been introduced on the java.net.http.HttpClient class:
/**
* Returns an {@link InetAddress} which will be used by this client
* to bind a socket when creating connections. If no
* {@link Builder#localAddress() local address} was set in the
* client's builder, then this method returns null, in which case
* the sockets created by this client will be bound to automatically
* assigned socket addresses.
*
* @return the local address that will be used by this client when
* creating connections
* @since 19
*/
public InetAddress localAddress() {
return null;
}
This method just returns whatever was set as the localAddress on the
Builder, using which this instance of HTTPClient was built.
(Internal) Implementation:
--------------------------
Within the java.net.http.HTTPClient implementation,
jdk.internal.net.http.HttpConnection is where the underlying
SocketChannel connections are handled. There are multiple sub-classes of
the jdk.internal.net.http.HttpConnection abstract class. However each of
those sub-classes ultimately ends up calling the
jdk.internal.net.http.PlainHttpConnection class for doing the actual
connection on the SocketChannel. The implementation in this class has
now been enhanced to take into account any localAddress that might have
been set on the HTTPClient. In the presence of such a configured
InetAddress, this implementation now calls a SocketChannel.bind() before
calling the SocketChannel.connect().
More precisely, the PlainHttpConnection.connectAsync(Exchange<?>
exchange) method has been enhanced to call the bind() before calling the
connect() on the channel. This PlainHttpConnection.connectAsync(), as
the name suggests, isn't supposed to have any blocking calls. My
knowledge in this area isn't vast, so I was unsure whether a
SocketChannel.bind() call is a blocking call, even when the
SocketChannel is configured to be non-blocking. The SocketChannel.bind()
doesn't state much about this semantic (unlike the
SocketChannel.connect()). Looking at the API and the implementation
(which ends up calling the native layer for the socket bind), I think a
call to bind() would be a blocking one. More so in the case where it
perhaps has to find a free/available port (when the passed port to the
socket address is 0, like in the implementation here). As such, in this
proposed implementation, I've considered SocketChannel.bind() to be
blocking and thus wrapped it around in a async call that needs to
complete successfully before an async connect is issued. I look forward
to inputs around this part on whether or not this is the right way to go
about it.
Testing:
-------
A new jtreg test has been introduced to test this new method. The test
is configured to run with both IPv6 and IPv4. tier1 testing with these
changes have passed on a personal run of GitHub Actions and a local test
run of "test/jdk/java/net/httpclient/" has also passed with these
changes. I'll run tier2 tests locally once we have finalized on the API
part of this change.
The PR is currently in draft and I will move it out of draft after the
API is finalized. I don't plan to rush this into JDK 18, so the new APIs
have all been marked for @since 19.
-Jaikiran
