Proposal - 8209137: Add ability to bind to specific local address to HTTP client

[Jaikiran Pai]

Hello Daniel,

On 03/12/21 5:59 pm, Daniel Fuchs wrote:
> 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().

Understood.

>
> 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.
>
That makes sense. I'll include this change in my updates.

> 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.

Will do.

>
> 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.

In my draft version, that security check and any security exception due 
that check was happening when the bind was actually issued. I see that 
currently for other security checks like the URLPermission check, the 
Exchange does those checks just before sending the request. I'll pay 
some more attention to this area to understand what would be a better 
place to do these checks for this new feature.

>
> 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.

If we do expose this as a SocketAddress, then in many cases where 
applications want to bind to an InetAddress of choice, they would almost 
always would have to send 0 as the port of the InetSocketAddress, isn't 
it? That way this address can be used to create multiple socket binds 
for different sockets/connections that might be needed by the client 
during different request executions. Any other constant port value would 
mean that at any given time only one connection bind would be successful 
out of it, isn't it? Would that mean that we would then enforce this 
port check for SocketAddress of type InetSocketAddress during the 
Builder.build()?

>
> > 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())

I'll leave this method out for now then.

>
> WRT to SocketChannel::bind I believe we should treat that as a non
> blocking call - as it shouldn't involve any network operation.

Thank you, that reasoning helped.

>
> 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.

That's a good point and like you say, we would need to include some text 
about when to use this method. I don't have the wording for that right 
now, but will think about it as I go along with the updates/changes.

Thank you very much for the inputs so far.

-Jaikiran