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
